Skip to content

Commit 314f331

Browse files
olexii4olexii4
authored
fix: minimal fix for airgap sample trust and factory URL source comparison (#1567) (#1568)
* fix: minimal fix for airgap sample trust and factory URL source comparison Four targeted changes to fix workspace source comparison and airgap sample trust for factory flows like: /load-factory?url=https%3A%2F%2Fregistry.devfile.io%2F...&storageType=per-user 1. Preferences/helpers.ts — automatically trust airgap sample URLs (matching /dashboard/api/airgap-sample/devfile/download) so the factory flow proceeds without requiring an explicit trust grant. Minor refactor: extract urlString before early-return guards. 2. ImportFromGit/helpers.ts — rename REVISION import to REVISION_ATTR (both are 'revision', so no functional change; aligns with the canonical constant name used in buildFactoryParams.ts). 3. buildFactoryParams.ts — apply decodeURIComponent() in getSourceUrl() so factoryParams.sourceUrl matches workspace.source. The factory hash double-encodes airgap URLs (%253F → %3F after first URLSearchParams decode), while workspace.source applies a second decode via URLSearchParams.get() (%3F → ?). Without this normalization the two sides differ and CheckExistingWorkspaces always creates a new workspace. 4. workspace-adapter/index.ts — replace split('&') + split('=')[1] with URLSearchParams.get('url') to extract the URL from the DevWorkspace annotation. The old approach truncated the value at the second '=' sign, breaking URLs whose query parameters contain '=' characters (e.g. base64-encoded values). URLSearchParams.get() also applies the same percent-decoding as point 3, keeping both sides consistent. Assisted-by: Claude Sonnet 4.6 * fix(typo): rename devfileSourse → devfileSource in workspace-adapter "Sourse" is a misspelling of "Source". Rename the two local variables devfileSourseStr and devfileSourse to devfileSourceStr and devfileSource. No functional change. Assisted-by: Claude Sonnet 4.6 * chore(license): add jsbn@0.1.1 to dev dependency excludes Assisted-by: Claude Sonnet 4.6 * fix: GitRepo component appends factory params to the Git repo URL (CRW-9659) The GitRepoFormGroup.getSource() method parsed the DevWorkspace factory annotation with split('&') + split('=') and then reconstructed the URL by appending every other factory param (che-editor, storageType, …). For airgap sample URLs this produced a malformed "Git repo URL" like: "…/download?id?che-editor=che-incubator/che-code/latest&storageType=per-user" Two bugs in the old parser: 1. split('=')[1] truncated the URL value at the second '=' sign. 2. All non-url params were re-joined and appended to the URL, adding factory orchestration params that have no place in the Git repo URL. Fix: replace the parser with URLSearchParams.get('url'). URLSearchParams.get() correctly handles '=' inside values (e.g. ?id=foo) and decodes percent-encoded characters (%3F → ?, %3D → =) so the URL shown in the Overview tab matches the clean devfile download URL. Add regression test: airgap sample URL with che-editor and storageType in the factory params — verifies that only the decoded 'url' value appears and factory params are never appended. Also stage a CSS line-wrap format fix in SamplesList/Gallery/Card. Assisted-by: Claude Sonnet 4.6 * fix: GitRepo URL truncated and double-'?' for airgap factory params (CRW-9659) The GitRepoFormGroup.getSource() method used split('=')[1] to extract the 'url' value from the factory annotation params string. For airgap sample URLs whose query string contains '=' characters (e.g. %3Fid%3Dnodejs-express, decoded by URLSearchParams to ?id=nodejs-express) this truncated the value at the second '=' and then appended other factory params with '?' as separator, producing a malformed double-'?' URL: "…/download?id?che-editor=che-incubator/che-code/latest&storageType=per-user" ^ missing =nodejs-express ^ wrong separator Fix: use URLSearchParams.get('url') to extract the full URL value. URLSearchParams handles '=' inside values correctly and decodes percent-encoded characters (%3F→?, %3D→=). The remaining factory params (editor-image, storageType, etc.) are still appended to represent the full workspace config; the separator is chosen based on whether the URL already has a '?'. Add regression test that verifies: - No double '?' in the resulting URL - Decoded query value (id=nodejs-express) is preserved - Factory params are appended with '&', not a second '?' Also commit a CSS line-wrap format fix in SamplesList/Gallery/Card. Assisted-by: Claude Sonnet 4.6 --------- Signed-off-by: Oleksii Orel <oorel@redhat.com>
1 parent 11f3186 commit 314f331

8 files changed

Lines changed: 124 additions & 47 deletions

File tree

‎.deps/EXCLUDED/dev.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ This file contains a manual contribution to .deps/dev.md and it's needed because
55
| `@eclipse-che/api@7.86.0` | ecd.che |
66
| `@eclipse-che/license-tool@2.0.0` | ecd.che |
77
| `axios@1.15.2` | [clearlydefined](https://clearlydefined.io/definitions/npm/npmjs/-/axios/1.15.2) |
8+
| `jsbn@0.1.1` | [clearlydefined](https://clearlydefined.io/definitions/npm/npmjs/-/jsbn/0.1.1) |
89
| `postcss@8.5.13` | [clearlydefined](https://clearlydefined.io/definitions/npm/npmjs/-/postcss/8.5.13) |
910

1011

‎packages/dashboard-frontend/src/components/ImportFromGit/helpers.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import {
2121
} from '@/components/WorkspaceProgress/CreatingSteps/Apply/Devfile/getGitRemotes';
2222
import { buildFactoryLoaderPath } from '@/preload/main';
2323
import { FactoryLocationAdapter } from '@/services/factory-location-adapter';
24-
import { REVISION } from '@/services/helpers/factoryFlow/buildFactoryParams';
24+
import { REVISION_ATTR } from '@/services/helpers/factoryFlow/buildFactoryParams';
2525

2626
const BR_NAME_REGEX = /^[0-9A-Za-z-./_]{1,256}$/;
2727

@@ -189,7 +189,7 @@ function setBranchToAzureDevOpsLocation(location: string, branch: string | undef
189189

190190
export function setBranchToLocation(location: string, branch: string | undefined): string {
191191
if (!FactoryLocationAdapter.isHttpLocation(location)) {
192-
return branch ? `${location}?revision=${branch}` : location;
192+
return branch ? `${location}?${REVISION_ATTR}=${branch}` : location;
193193
}
194194
const url = new URL(location);
195195
const pathname = url.pathname.replace(/^\//, '').replace(/\/$/, '');
@@ -326,7 +326,7 @@ export function getGitRepoOptionsFromLocation(location: string): {
326326
console.log(`Unable to get branch from '${location}'.${common.helpers.errors.getMessage(e)}`);
327327
}
328328
} else if (!FactoryLocationAdapter.isHttpLocation(location)) {
329-
gitBranch = searchParams.get(REVISION) || undefined;
329+
gitBranch = searchParams.get(REVISION_ATTR) || undefined;
330330
}
331331
return { location, gitBranch, remotes, devfilePath, hasSupportedGitService };
332332
}
@@ -449,7 +449,7 @@ export function setGitRepoOptionsToLocation(
449449
state.gitBranch = newOptions.gitBranch;
450450
}
451451
if (!FactoryLocationAdapter.isHttpLocation(location) && newOptions.gitBranch) {
452-
searchParams.set(REVISION, newOptions.gitBranch);
452+
searchParams.set(REVISION_ATTR, newOptions.gitBranch);
453453
}
454454
// update the location with the new gitBranch value
455455
let searchParamsStr = decodeURIComponent(searchParamsToString(searchParams));

‎packages/dashboard-frontend/src/pages/GetStarted/SamplesList/Gallery/Card/index.module.css‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@
1111
*/
1212

1313
.sampleCard {
14-
border: var(--pf-v6-c-card--BorderWidth) var(--pf-v6-c-card--BorderStyle) var(--pf-t--global--border--color--default);
14+
border: var(--pf-v6-c-card--BorderWidth) var(--pf-v6-c-card--BorderStyle)
15+
var(--pf-t--global--border--color--default);
1516
}
1617

1718
.sampleCardIcon {

‎packages/dashboard-frontend/src/pages/WorkspaceDetails/OverviewTab/GitRepo/__tests__/index.spec.tsx‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,44 @@ describe('GitRepoURL', () => {
126126
expect(screen.queryByRole('button', { name: copyButtonName })).toBeTruthy;
127127
expect(screen.queryByRole('button', { name: copyButtonNameAfter })).toBeFalsy;
128128
});
129+
130+
test('airgap sample URL: percent-encoded query string decoded correctly', async () => {
131+
// Regression for CRW-9659: the old split('=')[1] parser truncated the URL at the
132+
// second '=' sign. For an airgap URL stored as
133+
// url=http://…/download%3Fid%3Dnodejs-express&che-editor=…&storageType=per-user
134+
// it produced "…/download%3Fid%3Dnodejs-express?che-editor=…" — a double-'?' URL
135+
// with the '=nodejs-express' value missing.
136+
// URLSearchParams.get('url') correctly decodes %3F→? and %3D→=, giving
137+
// "…/download?id=nodejs-express", and appends other params with '&'.
138+
const devWorkspace = new DevWorkspaceBuilder()
139+
.withMetadata({
140+
name: 'test-workspace',
141+
annotations: {
142+
[DEVWORKSPACE_DEVFILE_SOURCE]: dump({
143+
factory: {
144+
params:
145+
'url=http://che-dashboard.eclipse-che.svc:8080/dashboard/api/airgap-sample/devfile/download%3Fid%3Dnodejs-express&che-editor=che-incubator%2Fche-code%2Flatest&storageType=per-user',
146+
},
147+
}),
148+
},
149+
})
150+
.build();
151+
152+
const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime });
153+
renderComponent(constructWorkspace(devWorkspace));
154+
155+
const copyButton = screen.getByRole('button', { name: 'Copy to clipboard' });
156+
await user.click(copyButton);
157+
158+
const copied = mockClipboard.mock.calls[0][0] as string;
159+
// The URL must not contain a double '?' — that was the symptom of the bug.
160+
expect(copied.split('?').length - 1).toBe(1);
161+
// The decoded query param must be preserved.
162+
expect(copied).toContain('id=nodejs-express');
163+
// Factory params are still included but appended with '&', not '?'.
164+
expect(copied).toContain('&che-editor=');
165+
expect(copied).toContain('&storageType=per-user');
166+
});
129167
});
130168

131169
function getComponent(workspace: Workspace) {

‎packages/dashboard-frontend/src/pages/WorkspaceDetails/OverviewTab/GitRepo/index.tsx‎

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -79,37 +79,43 @@ class GitRepoFormGroup extends React.PureComponent<Props, State> {
7979
};
8080
};
8181

82-
const factoryParams = devfileSource?.factory?.params;
83-
if (factoryParams === undefined) {
82+
const rawFactoryParams = devfileSource?.factory?.params;
83+
if (rawFactoryParams === undefined) {
8484
return source;
8585
}
8686

87-
const paramsArr = factoryParams.split('&');
88-
if (paramsArr.length === 0) {
87+
// Use URLSearchParams to correctly extract the 'url' value.
88+
// The old split('=')[1] approach truncated the URL at the second '=' character,
89+
// producing malformed display URLs for airgap sample URLs whose query string
90+
// contains '=' signs (e.g. %3Fid%3Dnodejs-express decoded to ?id=nodejs-express).
91+
const parsedParams = new URLSearchParams(rawFactoryParams);
92+
const urlValue = parsedParams.get('url');
93+
if (!urlValue) {
8994
return source;
9095
}
9196

92-
paramsArr.forEach(param => {
93-
const [key, value] = param.split('=');
94-
if (key === 'url') {
95-
if (!source.gitRepo && paramsArr.length === 1) {
96-
source.gitRepo = value;
97-
} else {
98-
source.gitRepo = value + '?' + source.gitRepo;
99-
}
100-
source.isUrl = new RegExp('^https?://').test(value);
101-
source.fieldName = source.isUrl ? new URL(value).pathname.replace(/^\//, '') : value;
102-
if (source.fieldName.length > 50) {
103-
source.fieldName = source.fieldName.substring(0, 50) + '...';
104-
}
105-
} else {
106-
if (source.gitRepo.length !== 0 && !source.gitRepo.endsWith('?')) {
107-
source.gitRepo = source.gitRepo + '&';
108-
}
109-
source.gitRepo = source.gitRepo + key + '=' + value;
97+
source.isUrl = /^https?:\/\//.test(urlValue);
98+
source.fieldName = source.isUrl ? new URL(urlValue).pathname.replace(/^\//, '') : urlValue;
99+
if (source.fieldName.length > 50) {
100+
source.fieldName = source.fieldName.substring(0, 50) + '...';
101+
}
102+
103+
// Reconstruct the full display URL: base URL + any additional factory params
104+
// (editor image, storage type, etc.) so the link reflects the full workspace config.
105+
const otherParams: string[] = [];
106+
parsedParams.forEach((value, key) => {
107+
if (key !== 'url') {
108+
otherParams.push(`${key}=${value}`);
110109
}
111110
});
112111

112+
if (otherParams.length === 0) {
113+
source.gitRepo = urlValue;
114+
} else {
115+
const separator = urlValue.includes('?') ? '&' : '?';
116+
source.gitRepo = urlValue + separator + otherParams.join('&');
117+
}
118+
113119
return source;
114120
}
115121

‎packages/dashboard-frontend/src/services/helpers/factoryFlow/buildFactoryParams.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,24 @@ export function buildFactoryParams(searchParams: URLSearchParams): FactoryParams
103103

104104
function getSourceUrl(searchParams: URLSearchParams): string {
105105
const devworkspaceResourcesUrl = getDevworkspaceResourcesUrl(searchParams);
106-
return devworkspaceResourcesUrl !== undefined
107-
? devworkspaceResourcesUrl
108-
: getFactoryUrl(searchParams);
106+
if (devworkspaceResourcesUrl !== undefined) {
107+
return devworkspaceResourcesUrl;
108+
}
109+
const factoryUrl = getFactoryUrl(searchParams);
110+
if (!factoryUrl) {
111+
return factoryUrl;
112+
}
113+
// Decode percent-encoded characters so this value matches workspace.source, which is
114+
// read via URLSearchParams.get() from the DevWorkspace annotation and therefore has
115+
// one additional level of decoding applied (e.g. %3F → ?, %3D → =).
116+
// This fixes comparison failures for airgap sample URLs that are double-encoded in
117+
// the factory hash (%253Fid%253Dnodejs-express → %3Fid%3Dnodejs-express after first
118+
// decode, then ?id=nodejs-express after the second decode in workspace.source).
119+
try {
120+
return decodeURIComponent(factoryUrl);
121+
} catch {
122+
return factoryUrl;
123+
}
109124
}
110125

111126
function getDevworkspaceResourcesUrl(searchParams: URLSearchParams): string | undefined {

‎packages/dashboard-frontend/src/services/workspace-adapter/index.ts‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -185,12 +185,12 @@ export class WorkspaceAdapter<T extends devfileApi.DevWorkspace> implements Work
185185
* It can be an HTTPS or SSH URL.
186186
*/
187187
get source(): string | undefined {
188-
const devfileSourseStr = this.workspace.metadata.annotations?.[DEVWORKSPACE_DEVFILE_SOURCE];
189-
if (!devfileSourseStr) {
188+
const devfileSourceStr = this.workspace.metadata.annotations?.[DEVWORKSPACE_DEVFILE_SOURCE];
189+
if (!devfileSourceStr) {
190190
return undefined;
191191
}
192192
// Parse the devfile source annotation to extract the repository URL
193-
const devfileSourse = load(devfileSourseStr) as {
193+
const devfileSource = load(devfileSourceStr) as {
194194
factory?: {
195195
params?: string;
196196
};
@@ -202,26 +202,27 @@ export class WorkspaceAdapter<T extends devfileApi.DevWorkspace> implements Work
202202
location?: string;
203203
};
204204
};
205-
// Check if the devfile source has a factory with parameters
206-
const factoryParams = devfileSourse?.factory?.params;
207-
if (factoryParams) {
208-
// Split the factory params string into an array of parameters
209-
const paramsArr = factoryParams.split('&');
210-
if (paramsArr.length > 0) {
211-
// Find the URL parameter in the factory params
212-
const targetParam = paramsArr.find(param => param.startsWith('url='));
213-
if (targetParam) {
214-
return targetParam.split('=')[1];
215-
}
205+
// Check if the devfile source has a factory with parameters.
206+
// Use URLSearchParams.get() so that '=' characters inside the URL value are
207+
// handled correctly (the old split('=')[1] approach truncates the value at the
208+
// second '=' which breaks URLs containing query parameters like ?id=foo).
209+
// URLSearchParams.get() also decodes percent-encoded characters (%3F → ?, %3D → =)
210+
// matching the decoding applied in buildFactoryParams.getSourceUrl().
211+
const rawFactoryParams = devfileSource?.factory?.params;
212+
if (rawFactoryParams) {
213+
const factoryParams = new URLSearchParams(rawFactoryParams);
214+
const location = factoryParams.get('url');
215+
if (location) {
216+
return location;
216217
}
217218
}
218219
// Check if the devfile source has a repository URL
219-
const repo = devfileSourse?.scm?.repo;
220+
const repo = devfileSource?.scm?.repo;
220221
if (repo) {
221222
return repo;
222223
}
223224
// Check if the devfile source has a URL location
224-
const location = devfileSourse?.url?.location;
225+
const location = devfileSource?.url?.location;
225226
if (location) {
226227
return location;
227228
}

‎packages/dashboard-frontend/src/store/Workspaces/Preferences/helpers.ts‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,18 +78,33 @@ function getRepoPattern(url: string): RegExp | undefined {
7878
}
7979
}
8080

81+
/**
82+
* Regex pattern to match airgap sample URLs that should be automatically trusted.
83+
* Matches URLs containing: /dashboard/api/airgap-sample/devfile/download
84+
* Examples:
85+
* - http://che-dashboard.eclipse-che.svc:8080/dashboard/api/airgap-sample/devfile/download?name=JBoss+EAP+8
86+
* - http://localhost:8080/dashboard/api/airgap-sample/devfile/download?id=java-lombok&che-editor=...
87+
*/
88+
export const AIRGAP_SAMPLE_PATTERN = /\/dashboard\/api\/airgap-sample\/devfile\/download/;
89+
8190
export function isTrustedRepo(
8291
trustedSources: api.TrustedSources | undefined,
8392
url: string | URL,
8493
): boolean {
94+
const urlString = url.toString();
95+
96+
// Always trust airgap sample URLs (internal samples)
97+
if (AIRGAP_SAMPLE_PATTERN.test(urlString)) {
98+
return true;
99+
}
100+
85101
if (trustedSources === undefined) {
86102
return false;
87103
}
88104
if (trustedSources === '*') {
89105
return true;
90106
}
91107

92-
const urlString = url.toString();
93108
const urlPattern = getRepoPattern(urlString);
94109
const urlRepo = extractRepo(urlString, urlPattern);
95110

0 commit comments

Comments
 (0)