-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: encode secret keys to support '/' in paths #5614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,6 +154,23 @@ describe.each([{ auth: AuthMode.JWT }, { auth: AuthMode.IDENTITY_ACCESS_TOKEN }] | |
| "TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQsIGNvbnNlY3RldHVyIGFkaXBpc2NpbmcgZWxpdC4gU2VkIGRvIGVpdXNtb2QgdGVtcG9yIGluY2lkaWR1bnQgdXQgbGFib3JlIGV0IGRvbG9yZSBtYWduYSBhbGlxdWEuIFV0IGVuaW0gYWQgbWluaW0gdmVuaWFtLCBxdWlzIG5vc3RydWQgZXhlcmNpdGF0aW9uCg==", | ||
| comment: "" | ||
| } | ||
| }, | ||
| { | ||
| path: "/", | ||
| secret: { | ||
| // Encoding the key simulates the actual HTTP request format | ||
| key: encodeURIComponent("path/to/secret"), | ||
| value: "slash-value", | ||
| comment: "Testing forward slashes in keys" | ||
| } | ||
|
Comment on lines
+162
to
+165
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test encodes the key at parse time, creating a literal percent-encoded string This test stores However, the actual use case — a secret key containing a real forward slash character — is never tested. To verify the feature works, the test data should use the raw slash-containing key, and the |
||
| }, | ||
| { | ||
| path: "/nested1/nested2/folder", | ||
| secret: { | ||
| key: encodeURIComponent("another/key/with/slashes"), | ||
| value: "nested-slash-value", | ||
| comment: "Testing slashes in nested paths" | ||
| } | ||
| } | ||
| ]; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -269,7 +269,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
|
|
||||||||||
| server.route({ | ||||||||||
| method: "GET", | ||||||||||
| url: "/:secretName", | ||||||||||
| url: "/*", | ||||||||||
| config: { | ||||||||||
| rateLimit: secretsLimit | ||||||||||
| }, | ||||||||||
|
|
@@ -284,7 +284,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| } | ||||||||||
| ], | ||||||||||
| params: z.object({ | ||||||||||
| secretName: z.string().trim().describe(RAW_SECRETS.GET.secretName) | ||||||||||
| "*": z.string().trim().describe(RAW_SECRETS.GET.secretName) | ||||||||||
| }), | ||||||||||
| querystring: z.object({ | ||||||||||
| projectId: z.string().trim().describe(RAW_SECRETS.GET.projectId), | ||||||||||
|
|
@@ -336,7 +336,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| projectId, | ||||||||||
| viewSecretValue: req.query.viewSecretValue, | ||||||||||
| path: secretPath, | ||||||||||
| secretName: req.params.secretName, | ||||||||||
| secretName: req.params["*"], | ||||||||||
| type: req.query.type, | ||||||||||
| includeImports: req.query.includeImports, | ||||||||||
| version: req.query.version | ||||||||||
|
|
@@ -351,7 +351,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| environment, | ||||||||||
| secretPath: req.query.secretPath, | ||||||||||
| secretId: secret.id, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| secretVersion: secret.version, | ||||||||||
| secretMetadata: secret.secretMetadata?.map((meta) => ({ | ||||||||||
| key: meta.key, | ||||||||||
|
|
@@ -383,7 +383,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
|
|
||||||||||
| server.route({ | ||||||||||
| method: "POST", | ||||||||||
| url: "/:secretName", | ||||||||||
| url: "/*", | ||||||||||
| config: { | ||||||||||
| rateLimit: secretsLimit | ||||||||||
| }, | ||||||||||
|
|
@@ -398,7 +398,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| } | ||||||||||
| ], | ||||||||||
| params: z.object({ | ||||||||||
| secretName: SecretNameSchema.describe(RAW_SECRETS.CREATE.secretName) | ||||||||||
| "*": SecretNameSchema.describe(RAW_SECRETS.CREATE.secretName) | ||||||||||
| }), | ||||||||||
| body: z.object({ | ||||||||||
| projectId: z.string().trim().describe(RAW_SECRETS.CREATE.projectId), | ||||||||||
|
|
@@ -449,7 +449,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| actorAuthMethod: req.permission.authMethod, | ||||||||||
| projectId: req.body.projectId, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| secretName: req.params.secretName, | ||||||||||
| secretName: req.params["*"], | ||||||||||
| type: req.body.type, | ||||||||||
| secretValue: req.body.secretValue, | ||||||||||
| skipMultilineEncoding: req.body.skipMultilineEncoding, | ||||||||||
|
|
@@ -471,7 +471,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| secretApprovalRequestSlug: secretOperation.approval.slug, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| environment: req.body.environment, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| eventType: SecretApprovalEvent.Create | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -490,7 +490,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| environment: req.body.environment, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| secretId: secret.id, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| secretVersion: secret.version, | ||||||||||
| secretMetadata: req.body.secretMetadata?.map((meta) => ({ | ||||||||||
| key: meta.key, | ||||||||||
|
|
@@ -522,7 +522,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
|
|
||||||||||
| server.route({ | ||||||||||
| method: "PATCH", | ||||||||||
| url: "/:secretName", | ||||||||||
| url: "/*", | ||||||||||
| config: { | ||||||||||
| rateLimit: secretsLimit | ||||||||||
| }, | ||||||||||
|
|
@@ -537,7 +537,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| } | ||||||||||
| ], | ||||||||||
| params: z.object({ | ||||||||||
| secretName: BaseSecretNameSchema.describe(RAW_SECRETS.UPDATE.secretName) | ||||||||||
| "*": BaseSecretNameSchema.describe(RAW_SECRETS.UPDATE.secretName) | ||||||||||
| }), | ||||||||||
| body: z.object({ | ||||||||||
| projectId: z.string().trim().describe(RAW_SECRETS.UPDATE.projectId), | ||||||||||
|
|
@@ -594,7 +594,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| environment: req.body.environment, | ||||||||||
| projectId: req.body.projectId, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| secretName: req.params.secretName, | ||||||||||
| secretName: req.params["*"], | ||||||||||
| type: req.body.type, | ||||||||||
| secretValue: req.body.secretValue, | ||||||||||
| skipMultilineEncoding: req.body.skipMultilineEncoding, | ||||||||||
|
|
@@ -620,7 +620,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| secretApprovalRequestSlug: secretOperation.approval.slug, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| environment: req.body.environment, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| eventType: SecretApprovalEvent.Update | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -639,7 +639,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| environment: req.body.environment, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| secretId: secret.id, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| secretVersion: secret.version, | ||||||||||
| secretMetadata: req.body.secretMetadata?.map((meta) => ({ | ||||||||||
| key: meta.key, | ||||||||||
|
|
@@ -670,7 +670,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
|
|
||||||||||
| server.route({ | ||||||||||
| method: "DELETE", | ||||||||||
| url: "/:secretName", | ||||||||||
| url: "/*", | ||||||||||
| config: { | ||||||||||
| rateLimit: secretsLimit | ||||||||||
| }, | ||||||||||
|
|
@@ -685,7 +685,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| } | ||||||||||
| ], | ||||||||||
| params: z.object({ | ||||||||||
| secretName: z.string().min(1).describe(RAW_SECRETS.DELETE.secretName) | ||||||||||
| "*": z.string().min(1).describe(RAW_SECRETS.DELETE.secretName) | ||||||||||
| }), | ||||||||||
| body: z.object({ | ||||||||||
| projectId: z.string().trim().describe(RAW_SECRETS.DELETE.projectId), | ||||||||||
|
|
@@ -719,7 +719,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| environment: req.body.environment, | ||||||||||
| projectId: req.body.projectId, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| secretName: req.params.secretName, | ||||||||||
| secretName: req.params["*"], | ||||||||||
| type: req.body.type | ||||||||||
| }); | ||||||||||
| if (secretOperation.type === SecretProtectionType.Approval) { | ||||||||||
|
|
@@ -734,7 +734,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| secretApprovalRequestSlug: secretOperation.approval.slug, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| environment: req.body.environment, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| eventType: SecretApprovalEvent.Delete | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -754,7 +754,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| environment: req.body.environment, | ||||||||||
| secretPath: req.body.secretPath, | ||||||||||
| secretId: secret.id, | ||||||||||
| secretKey: req.params.secretName, | ||||||||||
| secretKey: req.params["*"], | ||||||||||
| secretVersion: secret.version | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
@@ -1273,7 +1273,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
|
|
||||||||||
| server.route({ | ||||||||||
| method: "GET", | ||||||||||
| url: "/:secretName/reference-dependency-tree", | ||||||||||
| url: "/*/reference-dependency-tree", | ||||||||||
| config: { | ||||||||||
| rateLimit: secretsLimit | ||||||||||
| }, | ||||||||||
|
|
@@ -1288,7 +1288,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| } | ||||||||||
| ], | ||||||||||
| params: z.object({ | ||||||||||
| secretName: z.string().trim().describe(RAW_SECRETS.GET_REFERENCE_TREE.secretName) | ||||||||||
| "*": z.string().trim().describe(RAW_SECRETS.GET_REFERENCE_TREE.secretName) | ||||||||||
| }), | ||||||||||
| querystring: z.object({ | ||||||||||
| projectId: z.string().trim().describe(RAW_SECRETS.GET_REFERENCE_TREE.projectId), | ||||||||||
|
|
@@ -1308,7 +1308,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| }, | ||||||||||
| onRequest: verifyAuth([AuthMode.JWT]), | ||||||||||
| handler: async (req) => { | ||||||||||
| const { secretName } = req.params; | ||||||||||
| const { secretName } = req.params["*"], | ||||||||||
| const { secretPath, environment, projectId } = req.query; | ||||||||||
|
Comment on lines
+1311
to
1312
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Syntax error: destructuring from a string always yields
The same error occurs at line 1367 in the
Suggested change
|
||||||||||
|
|
||||||||||
| const { tree } = await server.services.secret.getSecretReferenceDependencyTree({ | ||||||||||
|
|
@@ -1328,7 +1328,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
|
|
||||||||||
| server.route({ | ||||||||||
| method: "GET", | ||||||||||
| url: "/:secretName/secret-reference-tree", | ||||||||||
| url: "/*/secret-reference-tree", | ||||||||||
| config: { | ||||||||||
| rateLimit: secretsLimit | ||||||||||
| }, | ||||||||||
|
|
@@ -1343,7 +1343,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| } | ||||||||||
| ], | ||||||||||
| params: z.object({ | ||||||||||
| secretName: z.string().trim().describe(RAW_SECRETS.GET_REFERENCE_TREE.secretName) | ||||||||||
| "*": z.string().trim().describe(RAW_SECRETS.GET_REFERENCE_TREE.secretName) | ||||||||||
| }), | ||||||||||
| querystring: z.object({ | ||||||||||
| projectId: z.string().trim().describe(RAW_SECRETS.GET_REFERENCE_TREE.projectId), | ||||||||||
|
|
@@ -1364,7 +1364,7 @@ export const registerSecretRouter = async (server: FastifyZodProvider) => { | |||||||||
| }, | ||||||||||
| onRequest: verifyAuth([AuthMode.JWT]), | ||||||||||
| handler: async (req) => { | ||||||||||
| const { secretName } = req.params; | ||||||||||
| const { secretName } = req.params["*"], | ||||||||||
| const { secretPath, environment, projectId } = req.query; | ||||||||||
| const { tree, value, secret } = await server.services.secret.getSecretReferenceTree({ | ||||||||||
| actorId: req.permission.id, | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,7 @@ export const useCreateSecretV3 = ({ | |
| skipMultilineEncoding, | ||
| tagIds | ||
| }) => { | ||
| const { data } = await apiRequest.post(`/api/v4/secrets/${secretKey}`, { | ||
| const { data } = await apiRequest.post(`/api/v4/secrets/${encodeURIComponent(secretKey)}`, { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. POST endpoint will still reject secret names containing The backend v4 POST route at export const SecretNameSchema = BaseSecretNameSchema.refine(
(el) => !el.includes(":") && !el.includes("/"),
"Secret name cannot contain colon or forward slash."
);The frontend correctly encodes the secret key as Impact: The fix works for GET, PATCH, and DELETE operations (which use permissive validators like To support creation: The POST route's |
||
| secretPath, | ||
| type, | ||
| environment, | ||
|
|
@@ -106,7 +106,7 @@ export const useUpdateSecretV3 = ({ | |
| skipMultilineEncoding, | ||
| secretMetadata | ||
| }) => { | ||
| const { data } = await apiRequest.patch(`/api/v4/secrets/${secretKey}`, { | ||
| const { data } = await apiRequest.patch(`/api/v4/secrets/${encodeURIComponent(secretKey)}`, { | ||
| projectId, | ||
| environment, | ||
| type, | ||
|
|
@@ -163,7 +163,7 @@ export const useDeleteSecretV3 = ({ | |
|
|
||
| return useMutation<object, object, TDeleteSecretsV3DTO>({ | ||
| mutationFn: async ({ secretPath = "/", type, environment, projectId, secretKey, secretId }) => { | ||
| const { data } = await apiRequest.delete(`/api/v4/secrets/${secretKey}`, { | ||
| const { data } = await apiRequest.delete(`/api/v4/secrets/${encodeURIComponent(secretKey)}`, { | ||
| data: { | ||
| projectId, | ||
| environment, | ||
|
|
@@ -443,7 +443,7 @@ export const useMoveSecrets = ({ | |
| }; | ||
|
|
||
| export const createSecret = async (dto: TCreateSecretsV3DTO) => { | ||
| const { data } = await apiRequest.post(`/api/v4/secrets/${dto.secretKey}`, dto); | ||
| const { data } = await apiRequest.post(`/api/v4/secrets/${encodeURIComponent(dto.secretKey)}`, dto); | ||
| return data; | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test coverage gap: Secret keys with
/are never exercisedThe
encodeURIComponentaddition is correct and defensive, but the test cases do not actually validate keys containing/. All secret names are generated viaformatSqlUsername()(e.g.,MYSQL_PASSWORD_ABCD1234), which only produces alphanumeric characters and underscores.To verify the fix works for its intended purpose, add a dedicated test case where a secret key contains
/(e.g., by passing a static key like"aws/parameter/store"instead of a formatted username) and assert that the secret retrieval succeeds end-to-end.