Skip to content

Commit 8705190

Browse files
olexii4sec-dummy
olexii4
and
sec-dummy
committed
fix: preserve branch info and extract factory parameters correctly
Fixes #23666 PROBLEM: -------- 1. Branch was not taken into account when creating workspace with editor definition Example: https://github.com/user/repo/tree/feature?che-editor=che-code Result: workspace created from 'main' instead of 'feature' 2. Factory and override parameters were removed from URL but not passed to backend Example: https://github.com/user/repo?override.devfileFilename=custom.yaml Result: devfile filename override was lost ROOT CAUSE: ----------- Factory parameters in query string prevented backend from correctly parsing branch information, and removed parameters were not extracted to overrideParams. SOLUTION: --------- Comprehensive URL sanitization with parameter extraction: 1. sanitizeUrlForFactoryResolver() - Returns { sanitizedUrl, extractedOverrideParams } - Extracts PROPAGATE_FACTORY_ATTRS to overrideParams - Extracts override.* parameters to overrideParams - Preserves branch info in URL path (/tree/branch) - For Azure DevOps: keeps version/path in URL AND extracts them - For SSH: extracts ALL params to overrideParams 2. getRepoFromLocation() - Extracts base repository URL without branches - Used for trusted sources comparison - Ensures any branch of trusted repo is recognized 3. factoryApi.ts - Uses sanitizeUrlForFactoryResolver() - Merges extracted params with existing overrideParams - Sends clean URL and complete overrideParams to backend BEHAVIOR EXAMPLES: ------------------ HTTP with override params: Input: https://github.com/user/repo?override.devfileFilename=custom.yaml&che-editor=che-code URL: https://github.com/user/repo Params: { 'override.devfileFilename': 'custom.yaml', 'che-editor': 'che-code' } HTTP with branch: Input: https://github.com/user/repo/tree/feature?che-editor=che-code URL: https://github.com/user/repo/tree/feature Params: { 'che-editor': 'che-code' } Azure DevOps (provider-specific params preserved): Input: https://dev.azure.com/org/proj/_git/repo?version=GBmain&che-editor=che-code URL: https://dev.azure.com/org/proj/_git/repo?version=GBmain Params: { 'version': 'GBmain', 'che-editor': 'che-code' } SSH (all params extracted): Input: git@github.com:user/repo.git?revision=main&override.devfileFilename=custom.yaml URL: git@github.com:user/repo.git Params: { 'revision': 'main', 'override.devfileFilename': 'custom.yaml' } SECURITY FIX: ------------- Implemented robust Azure DevOps domain detection using allowlist approach to prevent URL spoofing attacks (GitHub Advanced Security alert). isAzureDevOpsHost() with strict validation: - Exact match: dev.azure.com - Subdomain match: *.azure.com (requires >=3 parts, e.g., myorg.azure.com) - Legacy support: *.visualstudio.com (requires >=3 parts) - Blocks: azure.com alone, evil.azure.com.attacker.com, attacker.com/azure.com TESTING: -------- ✅ 88 tests for sanitizeUrlForFactoryResolver (includes new merging tests) ✅ 45 tests for getRepoFromLocation (includes security tests) ✅ 14 tests for factoryApi (updated for new behavior) ✅ 2274 total tests pass ✅ 215 snapshots pass ✅ All linting checks pass NEW FILES: ---------- - sanitizeUrlForFactoryResolver.ts (returns object with URL and params) - sanitizeUrlForFactoryResolver.spec.ts (88 tests) - getRepoFromLocation.ts (base repo extraction) - getRepoFromLocation.spec.ts (45 tests) - sanitizeLocation.ts (simple factory param removal) - sanitizeLocation.spec.ts (10 tests) MODIFIED FILES: --------------- - factoryApi.ts (uses new sanitization, merges params) - factoryApi.spec.ts (updated for extraction behavior) - helpers.ts (isTrustedRepo with getRepoFromLocation) - helpers.spec.ts (new tests for factory params) - UntrustedSourceModal/index.tsx (saves base repo only) Co-authored-by: GitHub Advanced Security <security@github.com> Signed-off-by: Oleksii Orel <oorel@redhat.com>
1 parent c6810b3 commit 8705190

File tree

15 files changed

+1578
-265
lines changed

15 files changed

+1578
-265
lines changed

‎packages/dashboard-frontend/src/components/UntrustedSourceModal/index.tsx‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import React from 'react';
2525
import { connect, ConnectedProps } from 'react-redux';
2626

2727
import { AppAlerts } from '@/services/alerts/appAlerts';
28+
import { getRepoFromLocation } from '@/services/helpers/factoryFlow/getRepoFromLocation';
2829
import { RootState } from '@/store';
2930
import { selectIsAllowedSourcesConfigured } from '@/store/ServerConfig/selectors';
3031
import { workspacePreferencesActionCreators } from '@/store/Workspaces/Preferences';
@@ -110,8 +111,8 @@ class UntrustedSourceModal extends React.Component<Props, State> {
110111
if (
111112
prevProps.isOpen === false &&
112113
this.props.isOpen === true &&
113-
(isTrusted === true || isAllowedSourcesConfigured) &&
114-
this.state.canContinue === true
114+
(isTrusted || isAllowedSourcesConfigured) &&
115+
this.state.canContinue
115116
) {
116117
this.handleContinue();
117118
}
@@ -163,7 +164,10 @@ class UntrustedSourceModal extends React.Component<Props, State> {
163164
} else if (trustAllCheckbox) {
164165
await this.props.addTrustedSource('*');
165166
} else {
166-
await this.props.addTrustedSource(location);
167+
// Extract clean repository URL without branches and factory parameters
168+
// This ensures we store only the base repo URL for comparison
169+
const repoUrl = getRepoFromLocation(location);
170+
await this.props.addTrustedSource(repoUrl);
167171
}
168172
}
169173

‎packages/dashboard-frontend/src/pages/GetStarted/SamplesList/__tests__/index.spec.tsx‎

Lines changed: 0 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -239,96 +239,6 @@ describe('Samples List', () => {
239239
);
240240
});
241241
});
242-
243-
describe('SSH URL handling', () => {
244-
const preferredPvcStrategy = 'per-workspace';
245-
246-
test('should handle SSH URL with .git extension', async () => {
247-
const sshUrl = 'git@github.com:eclipse-che/che-dashboard.git';
248-
const store = new MockStoreBuilder()
249-
.withBranding({
250-
docs: {
251-
storageTypes: 'storage-types-docs',
252-
},
253-
} as BrandingData)
254-
.withDwServerConfig({
255-
defaults: {
256-
pvcStrategy: preferredPvcStrategy,
257-
} as api.IServerConfig['defaults'],
258-
})
259-
.withDevfileRegistries({
260-
registries: {
261-
['registry-url']: {
262-
metadata: [
263-
{
264-
displayName: 'Test Sample SSH',
265-
description: 'Test sample with SSH URL',
266-
tags: ['Test'],
267-
icon: '/images/test.svg',
268-
links: {
269-
v2: sshUrl,
270-
},
271-
},
272-
],
273-
},
274-
},
275-
})
276-
.build();
277-
278-
renderComponent(store, editorDefinition, editorImage);
279-
280-
const sampleCardButton = screen.getByRole('button', { name: 'Select Sample' });
281-
await userEvent.click(sampleCardButton);
282-
283-
expect(mockWindowOpen).toHaveBeenCalledWith(
284-
`${origin}/dashboard/#/load-factory?url=git%40github.com%3Aeclipse-che%2Fche-dashboard.git&che-editor=che-incubator%2Fche-code%2Finsiders&editor-image=custom-editor-image&storageType=${preferredPvcStrategy}`,
285-
'_blank',
286-
);
287-
});
288-
289-
test('should handle SSH URL with revision parameter', async () => {
290-
const sshUrl = 'git@github.com:eclipse-che/che-dashboard.git?revision=test';
291-
const store = new MockStoreBuilder()
292-
.withBranding({
293-
docs: {
294-
storageTypes: 'storage-types-docs',
295-
},
296-
} as BrandingData)
297-
.withDwServerConfig({
298-
defaults: {
299-
pvcStrategy: preferredPvcStrategy,
300-
} as api.IServerConfig['defaults'],
301-
})
302-
.withDevfileRegistries({
303-
registries: {
304-
['registry-url']: {
305-
metadata: [
306-
{
307-
displayName: 'Test Sample SSH with Branch',
308-
description: 'Test sample with SSH URL and revision',
309-
tags: ['Test'],
310-
icon: '/images/test.svg',
311-
links: {
312-
v2: sshUrl,
313-
},
314-
},
315-
],
316-
},
317-
},
318-
})
319-
.build();
320-
321-
renderComponent(store, editorDefinition, editorImage);
322-
323-
const sampleCardButton = screen.getByRole('button', { name: 'Select Sample' });
324-
await userEvent.click(sampleCardButton);
325-
326-
expect(mockWindowOpen).toHaveBeenCalledWith(
327-
`${origin}/dashboard/#/load-factory?url=git%40github.com%3Aeclipse-che%2Fche-dashboard.git%3Frevision%3Dtest&che-editor=che-incubator%2Fche-code%2Finsiders&editor-image=custom-editor-image&storageType=${preferredPvcStrategy}`,
328-
'_blank',
329-
);
330-
});
331-
});
332242
});
333243

334244
function getComponent(

‎packages/dashboard-frontend/src/pages/GetStarted/SamplesList/index.tsx‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import { connect, ConnectedProps } from 'react-redux';
2525
import SamplesListGallery from '@/pages/GetStarted/SamplesList/Gallery';
2626
import SamplesListToolbar from '@/pages/GetStarted/SamplesList/Toolbar';
2727
import { ROUTE } from '@/Routes';
28-
import { FactoryLocationAdapter } from '@/services/factory-location-adapter';
2928
import {
3029
DEV_WORKSPACE_ATTR,
3130
EDITOR_ATTR,
@@ -98,20 +97,9 @@ class SamplesList extends React.PureComponent<Props, State> {
9897

9998
private async handleSampleCardClick(metadata: DevfileRegistryMetadata): Promise<void> {
10099
const { editorDefinition, editorImage } = this.props;
101-
102-
// Handle SSH URLs (git@...) and HTTP(S) URLs differently
103-
let factoryUrl: string;
104-
if (FactoryLocationAdapter.isSshLocation(metadata.links.v2)) {
105-
// SSH URLs should be used as-is
106-
factoryUrl = metadata.links.v2;
107-
} else {
108-
// HTTP(S) URLs need to be parsed and encoded properly
109-
const url = new URL(metadata.links.v2);
110-
factoryUrl = `${url.origin}${url.pathname}${encodeURIComponent(url.search)}`;
111-
}
112-
100+
const url = new URL(metadata.links.v2);
113101
const factoryParams: { [key: string]: string } = {
114-
[FACTORY_URL_ATTR]: factoryUrl,
102+
[FACTORY_URL_ATTR]: `${url.origin}${url.pathname}${encodeURIComponent(url.search)}`,
115103
};
116104

117105
const _editorDefinition = editorDefinition || this.props.defaultEditorId;

‎packages/dashboard-frontend/src/services/backend-client/__tests__/factoryApi.spec.ts‎

Lines changed: 37 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,15 @@ describe('Factory API', () => {
6565
{},
6666
);
6767

68+
// URL params are decoded during processing, version param stays in URL (but also extracted)
6869
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
69-
url: 'https://test.azure.com/_git/public-repo?version=GBtest/branch',
70+
url: 'https://test.azure.com/_git/public-repo?version=GBtest%2Fbranch',
71+
version: 'GBtest/branch',
7072
});
7173
expect(mockFetchParentDevfile).toHaveBeenCalled();
7274
});
7375

74-
it('should preserve revision parameter in HTTP URL and not extract to overrideParams', async () => {
76+
it('should extract revision factory parameter from HTTP URL to overrideParams', async () => {
7577
mockPost.mockResolvedValueOnce({
7678
data: factoryResolver,
7779
});
@@ -81,14 +83,16 @@ describe('Factory API', () => {
8183
{},
8284
);
8385

84-
// For HTTP URLs, revision stays in URL and is NOT added to overrideParams
86+
// For HTTP URLs, factory params (including revision) are extracted to overrideParams
87+
// This ensures clean URLs are sent to backend for proper branch detection
8588
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
86-
url: 'https://github.com/eclipse-che/che-dashboard.git?revision=my-branch',
89+
url: 'https://github.com/eclipse-che/che-dashboard.git',
90+
revision: 'my-branch',
8791
});
8892
expect(mockFetchParentDevfile).toHaveBeenCalled();
8993
});
9094

91-
it('should preserve all parameters in HTTP URL', async () => {
95+
it('should extract factory params and preserve non-factory params in HTTP URL', async () => {
9296
mockPost.mockResolvedValueOnce({
9397
data: factoryResolver,
9498
});
@@ -98,16 +102,20 @@ describe('Factory API', () => {
98102
{ existingParam: 'value' },
99103
);
100104

105+
// Factory param (revision) extracted to overrideParams
106+
// Non-factory params (sparse, df) stay in URL
107+
// Existing overrideParams preserved and merged
101108
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
102-
url: 'https://github.com/user/repo.git?revision=main&sparse=1&df=devfile.yaml',
109+
url: 'https://github.com/user/repo.git?sparse=1&df=devfile.yaml',
103110
existingParam: 'value',
111+
revision: 'main',
104112
});
105113
expect(mockFetchParentDevfile).toHaveBeenCalled();
106114
});
107115
});
108116

109117
describe('SSH locations', () => {
110-
it('should extract revision from SSH URL with only revision parameter (user example)', async () => {
118+
it('should extract revision from SSH URL to overrideParams', async () => {
111119
mockPost.mockResolvedValueOnce({
112120
data: factoryResolver,
113121
});
@@ -118,15 +126,15 @@ describe('Factory API', () => {
118126
{},
119127
);
120128

121-
// For SSH: ALL params extracted to overrideParams, URL is only the path
129+
// For SSH: ALL params extracted to overrideParams, URL is clean
122130
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
123131
url: 'git@github.com:svor/python-hello-world.git',
124132
revision: 'my-branch',
125133
});
126134
expect(mockFetchParentDevfile).toHaveBeenCalled();
127135
});
128136

129-
it('should extract all parameters from SSH URL to overrideParams', async () => {
137+
it('should extract all params from SSH URL to overrideParams', async () => {
130138
mockPost.mockResolvedValueOnce({
131139
data: factoryResolver,
132140
});
@@ -136,7 +144,7 @@ describe('Factory API', () => {
136144
{},
137145
);
138146

139-
// For SSH: ALL parameters go to overrideParams, URL has no query string
147+
// For SSH: ALL params (factory and non-factory) extracted to overrideParams
140148
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
141149
url: 'git@github.com:svor/python-hello-world.git',
142150
revision: 'my-branch',
@@ -146,7 +154,7 @@ describe('Factory API', () => {
146154
expect(mockFetchParentDevfile).toHaveBeenCalled();
147155
});
148156

149-
it('should extract parameters and merge with existing overrideParams', async () => {
157+
it('should merge extracted params with existing overrideParams', async () => {
150158
mockPost.mockResolvedValueOnce({
151159
data: factoryResolver,
152160
});
@@ -156,6 +164,7 @@ describe('Factory API', () => {
156164
{ someParam: 'value', anotherParam: 'test' },
157165
);
158166

167+
// Existing overrideParams preserved, URL params extracted and merged
159168
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
160169
url: 'git@github.com:svor/python-hello-world.git',
161170
someParam: 'value',
@@ -179,21 +188,22 @@ describe('Factory API', () => {
179188
expect(mockFetchParentDevfile).toHaveBeenCalled();
180189
});
181190

182-
it('should decode URL-encoded parameters correctly', async () => {
191+
it('should extract and decode URL parameters from SSH URL', async () => {
183192
mockPost.mockResolvedValueOnce({
184193
data: factoryResolver,
185194
});
186195

187196
await getFactoryResolver('git@github.com:user/repo.git?revision=feature%2Fmy-branch', {});
188197

198+
// revision extracted to overrideParams and decoded
189199
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
190200
url: 'git@github.com:user/repo.git',
191201
revision: 'feature/my-branch',
192202
});
193203
expect(mockFetchParentDevfile).toHaveBeenCalled();
194204
});
195205

196-
it('should override existing params with URL params', async () => {
206+
it('should merge URL params with existing overrideParams', async () => {
197207
mockPost.mockResolvedValueOnce({
198208
data: factoryResolver,
199209
});
@@ -204,17 +214,17 @@ describe('Factory API', () => {
204214
keepThis: 'value',
205215
});
206216

207-
// URL params should override existing overrideParams
217+
// URL params override existing overrideParams with same key
208218
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
209219
url: 'git@github.com:user/repo.git',
210-
revision: 'from-url',
211-
sparse: '2',
212-
keepThis: 'value',
220+
revision: 'from-url', // Overridden by URL param
221+
sparse: '2', // Overridden by URL param
222+
keepThis: 'value', // Preserved from initial overrideParams
213223
});
214224
expect(mockFetchParentDevfile).toHaveBeenCalled();
215225
});
216226

217-
it('should handle complex SSH URL with multiple parameters', async () => {
227+
it('should extract all parameters from complex SSH URL', async () => {
218228
mockPost.mockResolvedValueOnce({
219229
data: factoryResolver,
220230
});
@@ -224,7 +234,7 @@ describe('Factory API', () => {
224234
{},
225235
);
226236

227-
// All parameters extracted, URL is clean path only
237+
// ALL params extracted to overrideParams
228238
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
229239
url: 'git@gitlab.com:team/project.git',
230240
revision: 'develop',
@@ -235,17 +245,21 @@ describe('Factory API', () => {
235245
expect(mockFetchParentDevfile).toHaveBeenCalled();
236246
});
237247

238-
it('should handle SSH URL with only non-revision parameters', async () => {
248+
it('should extract override.* parameters from SSH URL', async () => {
239249
mockPost.mockResolvedValueOnce({
240250
data: factoryResolver,
241251
});
242252

243-
await getFactoryResolver('git@github.com:user/repo.git?sparse=1&df=devfile.yaml', {});
253+
await getFactoryResolver(
254+
'git@github.com:user/repo.git?revision=main&override.devfileFilename=custom.yaml',
255+
{},
256+
);
244257

258+
// ALL params including override.* extracted
245259
expect(mockPost).toHaveBeenCalledWith('/api/factory/resolver', {
246260
url: 'git@github.com:user/repo.git',
247-
sparse: '1',
248-
df: 'devfile.yaml',
261+
revision: 'main',
262+
'override.devfileFilename': 'custom.yaml',
249263
});
250264
expect(mockFetchParentDevfile).toHaveBeenCalled();
251265
});

‎packages/dashboard-frontend/src/services/backend-client/factoryApi.ts‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import axios from 'axios';
1414

1515
import { cheServerPrefix } from '@/services/backend-client/const';
1616
import { getParentDevfile } from '@/services/backend-client/parentDevfileApi';
17-
import { FactoryLocationAdapter } from '@/services/factory-location-adapter';
17+
import { sanitizeUrlForFactoryResolver } from '@/services/helpers/factoryFlow/sanitizeUrlForFactoryResolver';
1818
import { FactoryResolver } from '@/services/helpers/types';
1919

2020
export async function getFactoryResolver(
@@ -24,24 +24,18 @@ export async function getFactoryResolver(
2424
if (url.indexOf(' ') !== -1) {
2525
url = encodeURI(url);
2626
}
27-
// In the case of the Azure repository, the search parameters are encoded twice and need to be decoded.
28-
if (url.indexOf('?') !== -1) {
29-
const [path, search] = url.split('?');
30-
if (FactoryLocationAdapter.isHttpLocation(url)) {
31-
url = `${path}?${decodeURIComponent(search)}`;
32-
} else {
33-
// For SSH locations: extract ALL parameters to overrideParams and use only the path
34-
const searchParams = new URLSearchParams(decodeURIComponent(search));
35-
searchParams.forEach((value, key) => {
36-
overrideParams = { ...overrideParams, [key]: value };
37-
});
38-
// SSH URLs should not have query parameters - use only the path
39-
url = path;
40-
}
41-
}
27+
28+
// Sanitize URL by extracting factory-specific and override query parameters
29+
// This ensures the backend receives a clean URL for proper branch detection,
30+
// while still passing all necessary factory configurations via overrideParams.
31+
const { sanitizedUrl, extractedOverrideParams } = sanitizeUrlForFactoryResolver(
32+
url,
33+
overrideParams,
34+
);
35+
4236
const response = await axios.post(
4337
`${cheServerPrefix}/factory/resolver`,
44-
Object.assign({}, overrideParams, { url }),
38+
Object.assign({}, extractedOverrideParams, { url: sanitizedUrl }),
4539
);
4640

4741
const factoryResolver: FactoryResolver = response.data;

0 commit comments

Comments
 (0)