fix: support '/' in secret keys and updated docker to install bash#5604
fix: support '/' in secret keys and updated docker to install bash#5604AdeshGhadage wants to merge 2 commits intoInfisical:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Greptile SummaryThis PR introduces a Critical issues identified:
The core routing approach is conceptually sound, but implementation issues prevent safe merging. Confidence Score: 2/5
|
| const parts = secretKey.split("/"); | ||
| const actualSecretKey = parts.pop() as string; |
There was a problem hiding this comment.
Empty secret key when input ends with /
If a user enters a key with a trailing slash (e.g., "test/" or "/"), parts.pop() will return an empty string, making actualSecretKey = "". The resulting request URL becomes /api/v4/secrets/ (empty key segment), which hits an unintended endpoint or returns a confusing error.
Consider adding validation to guard against this case — either fall back to the original value so the server can return a proper validation error, or surface a clear client-side message before making the request.
| const splitKeyAndPath = (secretKey: string, secretPath: string = "/") => { | ||
| if (!secretKey.includes("/")) { | ||
| return { actualSecretKey: secretKey, actualSecretPath: secretPath }; | ||
| } | ||
| const parts = secretKey.split("/"); | ||
| const actualSecretKey = parts.pop() as string; | ||
| const folderPart = parts.join("/"); | ||
| const actualSecretPath = | ||
| secretPath === "/" | ||
| ? `/${folderPart}` | ||
| : `${secretPath}/${folderPart}`; | ||
| return { actualSecretKey, actualSecretPath }; | ||
| }; |
There was a problem hiding this comment.
Double slash when key starts with leading forward slash
If the input key begins with / (e.g., "/test/cred"), the function produces paths with double slashes:
For example, with key = "/test/cred" and path = "/":
- Split produces:
["", "test", "cred"] - After pop:
parts = ["", "test"], sofolderPart = "/test" - Result:
actualPath =/${folderPart}=//test`
When path = "/some":
actualPath = "/some//test" // double slash
These malformed paths will likely cause 404 errors or routing issues. Consider normalizing slashes:
| const splitKeyAndPath = (secretKey: string, secretPath: string = "/") => { | |
| if (!secretKey.includes("/")) { | |
| return { actualSecretKey: secretKey, actualSecretPath: secretPath }; | |
| } | |
| const parts = secretKey.split("/"); | |
| const actualSecretKey = parts.pop() as string; | |
| const folderPart = parts.join("/"); | |
| const actualSecretPath = | |
| secretPath === "/" | |
| ? `/${folderPart}` | |
| : `${secretPath}/${folderPart}`; | |
| return { actualSecretKey, actualSecretPath }; | |
| }; | |
| const actualSecretPath = | |
| secretPath === "/" | |
| ? `/${folderPart}`.replace(/\/+/g, "/") | |
| : `${secretPath}/${folderPart}`.replace(/\/+/g, "/"); |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Context
Fix #5598 handling of secret keys containing
/.Previously keys like
test/credproduced requests such as:POST /api/v4/secrets/test/credwhich caused a
404because/was interpreted as a URL path separator.This PR introduces a helper that splits keys containing
/into a folder path and the actual key name, appending the folder part to secretPath before sending the request.Example:
but One issue still remains: if the folder does not already exist, it will not be created and an error is shown saying the folder does not exist.
This PR also includes a Docker fix. Since
node:20.20.0-trixie-slimdoes not includebashby default, local development was failing. I addedbashinstallation in the Dockerfile to resolve this.Screenshots
Steps to verify the change
Mentioned in the issue.
Type
Checklist
type(scope): short description(scope is optional, e.g.,fix: prevent crash on syncorfix(api): handle null response).