fix(provider): validate coder_app URL scheme for external apps - #540
fix(provider): validate coder_app URL scheme for external apps#540mmustafasenoglu wants to merge 1 commit into
Conversation
When external=true, the URL field now requires a scheme (e.g. https://, vscode://, jetbrains-gateway://). Bare strings like "my-repo" or relative paths like "/some/path" are rejected because they cause JavaScript TypeError in the Coder frontend. Uses CustomizeDiff to cross-reference external and url fields. Internal URLs (external=false) still accept relative paths since they are resolved server-side. Fixes coder#483
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
Pull request overview
This PR adds validation to prevent invalid coder_app.url values for externally-opened apps (external = true), aligning provider behavior with JavaScript’s stricter URL parsing to avoid frontend crashes.
Changes:
- Added
CustomizeDiffvalidation incoder_appto require a URL scheme whenexternal = true. - Introduced
helpers.ValidateExternalURLfor scheme validation and added unit tests for it. - Added
coder_appacceptance/unit test cases covering external vs internal URL behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| provider/app.go | Adds CustomizeDiff validation for external app URLs. |
| provider/helpers/validation.go | Introduces ValidateExternalURL helper. |
| provider/helpers/validation_test.go | Adds unit tests for ValidateExternalURL. |
| provider/app_test.go | Adds test coverage for external/internal URL validation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| urlVal, ok := diff.GetOkExists("url") | ||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| u, err := url.Parse(urlVal.(string)) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL %q: %w", urlVal.(string), err) | ||
| } | ||
|
|
||
| if u.Scheme == "" { | ||
| return fmt.Errorf( | ||
| "\"url\" must have a URL scheme (e.g. https://, vscode://, jetbrains-gateway://) when \"external\" is true, got %q", | ||
| urlVal.(string), | ||
| ) | ||
| } |
| { | ||
| name: "localhost without scheme", | ||
| value: "localhost:8080", | ||
| label: "url", | ||
| expectError: false, // Go's url.Parse treats "localhost" as the scheme | ||
| }, |
| name: "ExternalLocalhostNoScheme", | ||
| url: "localhost:8080", | ||
| external: true, // Go's url.Parse treats "localhost" as the scheme, so this passes | ||
| }, |
| // Go's url.Parse is permissive and accepts bare strings like "my-repo" as | ||
| // relative URLs, but JavaScript's new URL() requires a scheme and will crash | ||
| // if one is not present. | ||
| func ValidateExternalURL(value any, label string) ([]string, []error) { |
There was a problem hiding this comment.
label is only used as a detail added to the error message, all the tests only pass "url" as a label . its unneeded as input to this function .
all first param returns are nil, I think these were meant to represent warnings but there are none. neither return parameter needs to be an array . mostly good tests tho.
could also check that parsed.Host is not empty .
untra
left a comment
untra
left a comment
There was a problem hiding this comment.
Thank you for the contribution. I am also actively working on this issue right now. there are improvements to validation and implementation, but good checking extra beyond url.Parse(value) ; external urls should carry a scheme and host.
Closes #483
Summary
Adds URL scheme validation for the
coder_appresource'surlfield whenexternal = true.Motivation
Go's
url.Parse()is permissive and accepts bare strings like"my-repo"as relative URLs. However, JavaScript'snew URL()in the Coder frontend requires a scheme and throwsTypeError: Invalid URLwhen encountering these values, crashing the UI.Changes
provider/app.go: AddedCustomizeDiffthat checksurlhas a scheme whenexternal = true. Internal URLs (external=false) still accept relative paths since they are resolved server-side.provider/helpers/validation.go: AddedValidateExternalURLhelper for standalone URL scheme validation.provider/helpers/validation_test.go: Added unit tests forValidateExternalURL.provider/app_test.go: AddedExternalURLValidationtest cases covering external/internal URLs with various schemes.Testing
All existing tests pass. New test cases cover: