Repository navigation
Commit b738dbe
authored
Merge commit from fork
* security: keep server-controlled content out of local command execution
Workspace, owner, and agent names, app commands, DERP region names, and
deployment URLs all come from the deployment. None of them may become
local command execution or trusted UI.
- Add `src/common/validation.ts`: an identifier grammar (a superset of
coderd's user, workspace, and agent names that excludes `.` and `--`,
the remote-authority separators), a safe-hostname check, and a
deployment URL check that rejects shell metacharacters and `%`.
- Validate identifiers when building and parsing remote authorities and
when composing the `owner/name` workspace identifier, which also
closes the `%h` expansion, Host-pattern, and `path.join` traversal
paths.
- Run workspace app commands over the SSH exec channel as argv
(`coder ssh [--tty] <workspace> -- <command>` through `shellPath` and
`shellArgs`) instead of typing a composed line into a local terminal
after a fixed delay. CLIs before 2.23.0 cannot take a command, so they
fail with a clear error rather than falling back. `--tty` is passed
from 2.34.0; earlier CLIs always allocate a PTY in command mode.
- Pass workspace operands after a `--` end-of-options separator in
start, update, speedtest, support bundle, and ping, and document the
CLI execution rules in AGENTS.md.
- Quote the `--url` and `--global-config` values in SSH `ProxyCommand`
with `escapeShellArg`, and reject deployment URLs carrying shell
metacharacters or `%` (an OpenSSH ProxyCommand token) before they are
embedded. The URL check also covers an `sh`-based ssh on Windows,
where the double-quoted form still expands. The user's own
`coder.globalFlags` keep their existing quoting so shell expansion in
them still works.
- Keep a known deployment's stored URL when a deep link names the same
origin, so a link cannot rewrite it without a prompt.
- Append the DERP region name to the network tooltip with
`MarkdownString.appendText`, and limit the tooltip's markdown trust to
the commands it links, instead of marking the whole string trusted.
Fixes SEC-1033, coder/security-assessments#28,
coder/security-assessments#27, coder/security-assessments#35,
coder/security-assessments#79, coder/security-assessments#42.
* security: name the invalid authority field and clarify host checks
Address review feedback:
- Throw per-field errors when parsing a remote authority instead of one
generic message, naming the invalid hostname, username, workspace, or
agent.
- Rename the safe-hostname helpers to the ShellSafe* family and add
validateShellSafeHostname, mirroring validateShellSafeUrl.
- Validate the deployment host in toRemoteAuthority, and explain why
quoting alone cannot neutralize the ProxyCommand URL.
* security: reject unsafe deployment URLs before sign-in
Validate the URL in maybeAskUrl, which every login prompt, auto-login,
and vscode:// link passes through, so a bad URL fails before
authentication instead of at the SSH ProxyCommand. The ProxyCommand
check stays for URLs that reach storage another way.
Normalize the URL with `new URL` first so international domain names
are stored in their ASCII form and pass the check.
Explain why the hostname check is stricter than the URL check.
* fix(security): separate hostname safety and report validation failures safely
Validate filesystem-safe deployment hostnames separately from SSH safety, applying Windows-specific restrictions only on Windows.
Keep rejected values out of notifications, provide Coder Output diagnostics, preserve supported legacy agent names, and retain credentials when validation prevents migration.
* fix(security): ignore unsafe stored URLs and reject URL credentials
Stored session, current deployment, and URL history entries whose URL
fails the shell-safe check are now ignored. A URL planted by an older
version prompts a fresh sign-in instead of failing every connection,
and a correct link can replace it.
Sign-in also rejects a username, password, query, or fragment in the
deployment URL. Credentials would be written to the SSH config in plain
text, and a query or fragment breaks the API paths built on the URL.
* fix(security): apply one deployment URL check at sign-in and storage reads
Replace the separate credential, query, and shell-character checks with
`validateDeploymentUrl` and `isValidDeploymentUrl`, so sign-in, the SSH
ProxyCommand, and stored session, deployment, and URL history reads all
apply the same rules. Stored URLs with credentials, a query, or a
fragment are now ignored instead of used.
When a stored session URL can't be parsed, the hostname mismatch error
no longer includes the URL, which can contain credentials and is logged.
Share one list of invalid URLs across the tests for each layer, and
check that auth values and the header command stay literal in the
ProxyCommand on each platform.
Addresses SEC-1120 and SEC-1121.1 parent 53dd0ed commit b738dbe
39 files changed
Lines changed: 1203 additions & 114 deletions
File tree
- src
- api
- cli
- common
- error
- remote
- settings
- storage
- ui
- uri
- vscode
- test
- mocks
- unit
- api
- cli
- common
- remote
- settings
- storage
- ui
- uri
- vscode
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
64 | 79 | | |
65 | 80 | | |
66 | 81 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
27 | 50 | | |
28 | 51 | | |
29 | 52 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| 9 | + | |
| 10 | + | |
9 | 11 | | |
10 | 12 | | |
11 | 13 | | |
12 | 14 | | |
13 | 15 | | |
14 | | - | |
15 | | - | |
16 | | - | |
| 16 | + | |
17 | 17 | | |
18 | | - | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
19 | 21 | | |
20 | 22 | | |
21 | 23 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| 81 | + | |
81 | 82 | | |
82 | 83 | | |
83 | 84 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
6 | | - | |
| 6 | + | |
7 | 7 | | |
8 | 8 | | |
9 | | - | |
10 | | - | |
11 | | - | |
12 | | - | |
13 | | - | |
| 9 | + | |
| 10 | + | |
14 | 11 | | |
15 | 12 | | |
16 | 13 | | |
| |||
83 | 80 | | |
84 | 81 | | |
85 | 82 | | |
86 | | - | |
| 83 | + | |
87 | 84 | | |
88 | 85 | | |
89 | 86 | | |
| 87 | + | |
90 | 88 | | |
91 | 89 | | |
92 | 90 | | |
| |||
134 | 132 | | |
135 | 133 | | |
136 | 134 | | |
137 | | - | |
138 | | - | |
139 | 135 | | |
140 | 136 | | |
141 | 137 | | |
142 | 138 | | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
143 | 142 | | |
144 | 143 | | |
145 | 144 | | |
| |||
156 | 155 | | |
157 | 156 | | |
158 | 157 | | |
159 | | - | |
| 158 | + | |
160 | 159 | | |
161 | 160 | | |
162 | 161 | | |
163 | 162 | | |
164 | 163 | | |
165 | | - | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
166 | 167 | | |
167 | | - | |
168 | | - | |
169 | | - | |
170 | | - | |
171 | | - | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
176 | | - | |
177 | | - | |
178 | | - | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
179 | 178 | | |
180 | | - | |
181 | | - | |
182 | | - | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
183 | 190 | | |
184 | 191 | | |
185 | 192 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1105 | 1105 | | |
1106 | 1106 | | |
1107 | 1107 | | |
1108 | | - | |
| 1108 | + | |
| 1109 | + | |
1109 | 1110 | | |
1110 | 1111 | | |
1111 | 1112 | | |
1112 | | - | |
| 1113 | + | |
1113 | 1114 | | |
1114 | 1115 | | |
1115 | 1116 | | |
1116 | | - | |
| 1117 | + | |
1117 | 1118 | | |
1118 | 1119 | | |
1119 | 1120 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
31 | 34 | | |
32 | 35 | | |
33 | 36 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
| 4 | + | |
| 5 | + | |
4 | 6 | | |
5 | 7 | | |
6 | 8 | | |
7 | 9 | | |
8 | | - | |
| 10 | + | |
9 | 11 | | |
10 | 12 | | |
11 | | - | |
| 13 | + | |
12 | 14 | | |
13 | 15 | | |
14 | 16 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
147 | 147 | | |
148 | 148 | | |
149 | 149 | | |
150 | | - | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
151 | 154 | | |
152 | 155 | | |
153 | 156 | | |
| |||
0 commit comments