feat(secrets): validate device identifier format before use in paths - #2874
Open
rsdmike wants to merge 1 commit into
Open
feat(secrets): validate device identifier format before use in paths#2874rsdmike wants to merge 1 commit into
rsdmike wants to merge 1 commit into
Conversation
Device identifiers arriving on the websocket were only checked for length. Add a format check so identifiers are limited to alphanumerics, dots, hyphens, and underscores before they are used to build secret-provider and MPS device paths. The format check is kept separate from the length check so a future identifier scheme can change the expected length without affecting it. A matching guard in VaultService validates the path it receives so a bad value surfaces immediately.
There was a problem hiding this comment.
Pull request overview
This PR hardens handling of device identifiers received over the RPC WebSocket by validating the identifier format before it’s used to construct secret-provider (Vault) paths, and adds a Vault-side guard to fail fast on unsafe secret paths.
Changes:
- Add a
SAFE_IDENTIFIERregex check (plus a consecutive-dot guard) toValidator.verifyPayloadforpayload.uuid. - Add a
assertSafeSecretPathguard inVaultServiceand call it before all Vault secret operations. - Add/extend unit tests covering valid UUIDs and multiple invalid-character/path-injection patterns.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Validator.ts | Adds identifier-format validation for payload.uuid beyond length checks. |
| src/validator.test.ts | Adds tests for valid UUID and invalid-character UUID inputs. |
| src/secrets/vault/index.ts | Introduces assertSafeSecretPath and applies it to Vault secret operations. |
| src/secrets/vault/index.test.ts | Adds coverage for assertSafeSecretPath allow/deny behavior and provider rejection of malformed paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+20
to
+24
| path.startsWith('/') || | ||
| path.includes('#') || | ||
| path.includes('?') || | ||
| path.includes('\\') || | ||
| path.includes('%') || |
| import { type Configurator } from './Configurator.js' | ||
| import { type DeviceCredentials } from './interfaces/ISecretManagerService.js' | ||
|
|
||
| // device identifiers are alphanumerics, dots, hyphens, and underscores |
Comment on lines
+233
to
+237
| ['../profiles/default#xxxxxxxxxxxxxxxx', 'slash and hash'], | ||
| ['../certs/acm-domain#xxxxxxxxxxxxxxxx', 'slash characters'], | ||
| ['..%2fprofiles%2fdefault#xxxxxxxxxxxx', 'percent character'], | ||
| ['4bac9510-04a6-4321-bae2-d45ddf07b6#4', 'hash character'], | ||
| ['4bac9510-04a6-4321-bae2-d45ddf07b6?4', 'question mark'] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Device identifiers arriving on the websocket were only checked for length. Add a format check so identifiers are limited to alphanumerics, dots, hyphens, and underscores before they are used to build secret-provider and MPS device paths.
The format check is kept separate from the length check so a future identifier scheme can change the expected length without affecting it. A matching guard in VaultService validates the path it receives so a bad value surfaces immediately.
PR Checklist
What are you changing?
Anything the reviewer should know when reviewing this PR?
If the there are associated PRs in other repositories, please link them here (i.e. device-management-toolkit/repo#365 )