feat: add user secrets client utilities#25370
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review (Netero). 1 P2, 1 P3.
Clean utility layer with thorough test coverage (~580 lines production, ~639 lines test). Validation logic, payload builders, and import parsers are well-structured. Two mechanical issues to address before full panel review.
This is a first-pass review only: these are mechanical findings from Netero. The full review panel has not yet reviewed this PR and will review after these findings are addressed.
"The specifier field in the lockfile must match the range declared in package.json." — Netero, sweating the small stuff so CI doesn't have to
🤖 This review was automatically generated with Coder Agents.
afe1534 to
d6ed353
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (R2). 1 P2, 5 P3, 1 Nit. DEREM-1 and DEREM-2 from R1 verified fixed.
Well-structured utility layer. Backend/frontend validation alignment is thorough: reserved env names, prefixes, CODER namespace, file path rules, byte limits, and POSIX regex all match codersdk/usersecretvalidation.go exactly. Import parsers handle .env, JSON, and YAML with proper type checking and the isRecord prototype guard is good defense-in-depth against parsed YAML/JSON prototype pollution. Test coverage ratio is solid (1.2:1 test-to-code) and encodeURIComponent usage is correct and verified with double-encoding tests. The importUserSecretsSequential error-per-secret design is the right call for partial imports.
The main gap is the .env export prefix (DEREM-3), which will trip users on their first import attempt. The import validation hole (DEREM-4) is the other structural issue: three of four field validators run during import, but value validation is skipped.
PS. Three reviewers independently noted that route-unsafe name validation (/?#) exists only on the frontend. The backend accepts any non-empty name, and the Go SDK constructs URLs without encoding, so a secret created via CLI with name foo/bar becomes unmanageable via REST. The frontend is correct here; the backend needs the matching guard. Not actionable in this PR but worth a follow-up ticket.
"A dialog opening in the wrong view is P1, not P3. But an export keyword opening in the wrong parser? That's at least a P2." - Hisoka, who would know about misdirection
🤖 This review was automatically generated with Coder Agents.
| return results; | ||
| }; | ||
|
|
||
| function parseEnvFile(content: string): CreateUserSecretRequest[] { |
There was a problem hiding this comment.
In the interest of simplifying/focusing on the MVP, we could trim out everything related to bulk import. It's not 100% required for beta and something we could add in a follow-up PR. Might help keep the initial stack with the critical pieces less noisy. cc @matifali
There was a problem hiding this comment.
I'm not opposed to this, there is a lot to review here.
That being said, I'm still in favor of including it given that it's already with these PRs and working well according to my manual testing
| const routeUnsafeSecretNameRegex = /[/?#]/; | ||
|
|
||
| // Env name, file path, and value validation rules mirror codersdk/usersecretvalidation.go. | ||
| // Keep these rules in sync with the backend validators. |
There was a problem hiding this comment.
We should add a batch API on the backend rather than duping all this validation in frontend code. This has a lot of potential for inconsistencies.
| ); | ||
| }; | ||
|
|
||
| export const importUserSecretsSequential = async ( |
There was a problem hiding this comment.
It seems like this could result in partial bulk creation. I think we probably want this to function as "all or nothing" since the user is providing a number of secrets in some file format. The UX seems a bit awkward a user has to retry an upload where 1/N failed to be created for whatever reason.
Seems like another point in favor of a backend API for this.
There was a problem hiding this comment.
Yeah a good point. I'm happy to rework this stack, removing the bulk upload for now and make a new ticket for that.
Adds the frontend client pieces for user secrets: API methods, MSW fixtures, form validation, and
.env, JSON, and YAML import parsing.This is the base of the user secrets settings page stack.
This PR was generated by Coder Agents.