@W-21182296 fix: make webapplication.json optional#1688
Conversation
cb14d90 to
c7d2de7
Compare
| ]); | ||
| } | ||
|
|
||
| let config: unknown; |
There was a problem hiding this comment.
We should create named types for the config.
There was a problem hiding this comment.
+1. If you use named types, then you can do typeguard assertions.
|
|
||
| // Report all unknown properties at once so the developer can fix them in a single pass. | ||
| const disallowed = keys.filter((k) => !ALLOWED_TOP_LEVEL.has(k)); | ||
| if (disallowed.length > 0) { |
There was a problem hiding this comment.
Local dev will have some properties in the config that are not included in this validation. I don't think we need to prevent deployment due to unused properties being set.
There was a problem hiding this comment.
if the property isn't allowed, we should throw an error because the deploy would fail server-side anyway. We don't check for unused props here.
| } | ||
|
|
||
| const hasFileUnder = (dirPath: SourcePath, depth = 0): boolean => { | ||
| if (depth > MAX_RECURSION_DEPTH) return false; |
There was a problem hiding this comment.
| if (depth > MAX_RECURSION_DEPTH) return false; | |
| if (depth >= MAX_RECURSION_DEPTH) return false; |
| const raw = this.tree.readFileSync(descriptorPath); | ||
| validateWebApplicationJson(raw, descriptorPath, contentPath, this.tree); | ||
| } catch (e) { | ||
| if (e instanceof Error && e.message === 'Method not implemented') { |
There was a problem hiding this comment.
I'm not sure that I understand this error condition. If validation throws Method not implemented, the config is considered valid? Is this correct?
There was a problem hiding this comment.
During retrieve, the tree is a zip. Zip trees don't support readFileSync, so it throws. We catch that and skip validation — the config in the org was already validated server-side when it was deployed.
- Make webapplication.json optional (file-based routing when absent) - Skip client-side validation during retrieve (ZipTreeContainer) - Add containsPathTraversal() rejecting .., absolute paths, null bytes, control chars, glob wildcards, backslashes, and percent-encoding - Add assertNoTraversal() resolved-path check as defense in depth - Add 100 KB size limit matching server-side WebApplicationFileProcessor - Validate fallback/rewrite file existence with and without outputDir - Reject outputDir that resolves to the bundle root (. or ./) - Config paths must use forward slashes (URL-style, portable) Co-authored-by: Cursor <cursoragent@cursor.com>
trimPathToContent splits by path.sep, so zip-style forward-slash paths break on Windows. Use path.join to produce native separators. Co-authored-by: Cursor <cursoragent@cursor.com>
bd9ef6b to
384ca1b
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
| if (value === null) return 'null'; | ||
| if (Array.isArray(value)) return 'array'; |
There was a problem hiding this comment.
Nitpick: I dislike single-line if statements. Can you change these to use braces?
| function configError(message: string, actions?: string[]): SfError { | ||
| return new SfError(message, 'InvalidWebApplicationConfigError', actions); | ||
| } | ||
|
|
||
| function fileError(message: string, actions?: string[]): SfError { | ||
| return new SfError(message, 'ExpectedSourceFilesError', actions); | ||
| } |
There was a problem hiding this comment.
Nitpick: Could these methods be called createXError instead of xError, so that it's immediately clear in their invocations that they're functions and not classes?
| if (DOT_DOT_SEGMENT.test(value)) return 'path traversal (..)'; | ||
| if (value === '..') return 'path traversal (..)'; | ||
| if (value.startsWith('/') || value.startsWith('\\')) return 'absolute path'; | ||
| if (value.includes('\0')) return 'null byte'; | ||
| for (let i = 0; i < value.length; i++) { | ||
| if (value.charCodeAt(i) < 0x20) return 'control character'; | ||
| } | ||
| if (value.includes('*') || value.includes('?')) return 'glob wildcard'; | ||
| if (value.includes('\\')) return 'backslash (use forward slashes)'; | ||
| if (value.includes('%')) return 'percent-encoding'; |
There was a problem hiding this comment.
Nitpick: Single-line if statements.
| `webapplication.json '${configKey}' value "${value}" contains ${reason}. Config paths must use forward slashes.`, | ||
| [`Fix "${value}": use relative paths with forward slashes only, no special characters.`] |
There was a problem hiding this comment.
You should put these strings in the messages.md file instead of defining them here. Same goes for other user-facing text.
| ]); | ||
| } | ||
|
|
||
| let config: unknown; |
There was a problem hiding this comment.
+1. If you use named types, then you can do typeguard assertions.
| descriptorPath: string, | ||
| contentPath: SourcePath, | ||
| tree: TreeContainer | ||
| ): void { |
There was a problem hiding this comment.
Is it possible for this method to be split into smaller sub-methods? It's just a little bulky as-is. Additionally, does it maybe make sense for this code to be moved into a new file altogether?
This PR makes webapplication.json optional for WebApplication bundles and adds client-side validation with path security checks when the file is present.
What changed:
Previously, deploying a WebApplication without a webapplication.json descriptor would fail. Now it's treated as file-based routing and succeeds — only the -meta.xml is required.
When webapplication.json is present, it's validated before deploy: we check for valid JSON, enforce the allowed schema (apiVersion, outputDir, routing, headers), verify types and enum values, and confirm that referenced files (outputDir, fallback, rewrite targets) actually exist in the bundle. Fallback and rewrite file-existence checks run even when outputDir is not set, resolving paths relative to the bundle root. Error messages include the specific problem, the received value, and a suggested fix.
Path values in the descriptor go through security validation matching the server-side WebApplicationFileProcessor.java. A containsPathTraversal() check rejects .. segments, absolute paths, null bytes, control characters, glob wildcards, backslashes, and percent-encoding. A secondary resolved-path check (assertNoTraversal) acts as defense in depth, ensuring no path escapes its parent directory after resolution. An outputDir that resolves to the bundle root itself (e.g. . or ./) is also rejected. Config paths must use forward slashes — they're bundle-relative and URL-style, matching the server's pure-string validation logic. The descriptor is capped at 100 KB, matching the server-side limit.
During retrieve, validation is skipped since the org already validated the metadata server-side. This also fixes a crash where ZipTreeContainer.readFileSync (not implemented) would throw during retrieve.
The filePathGenerator was updated to only return the meta XML for WebApplication — since the descriptor is optional, it shouldn't be listed as a guaranteed file path.
@W-21182296@ or @W-12345678@