test(api): lock security-critical behaviour, then small guarded fixes#243
Merged
Conversation
The guards that prevent cross-tenant access had zero tests, which is the highest-risk gap in the backend: a silent regression here would let one account reach another account's devices or api keys. These specs pin the current authz contract without changing any code: - AuthGuard: valid bearer resolves the user; invalid/expired bearer is 401; a valid x-api-key resolves via findActiveApiKeyByClientKey plus a matching bcrypt hash and attaches request.apiKey; a non-matching hash, an unknown key, and no credentials are all rejected; a token that resolves an id for a user that no longer exists is 401. - CanModifyDevice / CanModifyApiKey: owner passes; non-owner is rejected (the cross-tenant case); admin passes regardless; an invalid ObjectId is 400 before any lookup; a missing record is rejected. Guards are plain constructor-injected classes, so they are instantiated directly with mocked services and a minimal ExecutionContext, which is lighter than a testing module and equally faithful. 17 tests, all passing against the current unchanged guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
auth.service was 521 lines with only the withoutPassword helper covered. These specs pin the flows a regression would hurt most, all against the current unchanged code: - validateEmail / validatePassword: boundary and malformed-input cases, including the 6 and 128 character password edges. - findActiveApiKeyByClientKey: the exact-masked hit path and the legacy prefix-regex fallback, asserting the revoked-key exclusion is applied on both lookups. This is the behaviour lock for the upcoming regex-escape fix: it captures how a legitimate key resolves today. - generateApiKey: the raw key is returned once, and only a masked value plus a bcrypt hash of the key are persisted (never the raw key). - changePassword: a wrong old password is rejected without saving; the success path replaces the stored hash. - resetPassword: a missing or non-matching OTP is rejected without saving; the success path updates the password and closes the reset window. Note (to be fixed under the refactor part, not here): changePassword calls validatePassword without await, so the length rule does not actually block a weak password on change. Left as-is in this behaviour -lock commit and tracked for a guarded fix. 16 tests, all passing against current code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
webhook.service was 1039 lines with zero tests. These specs pin the parts a regression would hurt most, without changing any code: - validateDeliveryUrl: accepts normal https; rejects non-http(s) and malformed URLs; rejects loopback and private hosts including the cloud metadata IP. This locks the SSRF guard so it cannot silently regress. - Signing secret validation: a secret under 20 characters is rejected on both create and update. - attemptWebhookDelivery signing: the X-Signature header equals an independently computed HMAC-SHA256 of the JSON payload under the subscription secret, and the signature changes with the secret. - Delivery abort: when the subscription is inactive or soft-deleted, the attempt is marked aborted and saved and no HTTP request is made. axios is mocked so no request leaves the process; private methods are exercised through the instance. 16 tests, all passing against current code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
support.service had no tests. These lock both flows against current code: - createSupportMessage: the 24h rate limit rejects a fourth request without persisting or emailing; the success path saves the message, strips the turnstile token before persistence, emails the requester, and returns success. - requestAccountDeletion: a missing or invalid user id and an unknown user are both NotFound; a second request is a Conflict; the success path records accountDeletionRequestedAt with the reason and emails the user. 6 tests, all passing against current code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
validateEmail and validatePassword are async and signal failure by throwing (a rejected promise). Three callers invoked them without await: register (both) and changePassword. An unawaited rejected promise is dropped, so execution continued past the check: registration accepted a malformed email or an out-of-range password, and a password change accepted a too-short new password. The validators were effectively dead. Adding await makes the existing rules take effect at their intended point. Legitimate clients are unaffected: the web signup and change-password forms already enforce a valid email and the length bounds, so this only rejects input the service was always meant to reject, with a clean 400 instead of persisting bad data. Guards were written first and seen to fail against the pre-fix code (register with a bad email or short password still created the user; changePassword with a short password still saved), then pass after the await is added. The valid-input paths are asserted to still succeed. This is the dependency-free half of what a global ValidationPipe would have covered. The pipe itself is deferred: class-transformer is not installed, so enabling it now would add a dependency and risk crashing once any DTO gains a decorator. Recommended as a separate follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
findActiveApiKeyByClientKey builds a fallback lookup as
new RegExp(`^${prefix}`) from the client-supplied key prefix and runs it
as a Mongo $regex. A prefix containing regex metacharacters compiles to
the wrong pattern or, for something like "((((", throws SyntaxError and
fails the request. On the authentication hot path that is a denial and a
correctness hole.
The prefix is now escaped with escapeRegExp before it reaches the RegExp.
escapeRegExp already existed under gateway with its own spec; it is moved
to src/common so auth and gateway share one copy, and the gateway import
is updated. No behaviour change for the gateway caller.
Guarded by the existing lock (a legitimate key still resolves via the
masked hit and the prefix fallback) plus a new case, written first and
seen to fail against the pre-fix code (it threw "Unterminated group"):
a key with metacharacters now compiles to a literal pattern and resolves
to no match instead of throwing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getDevicesForUser did find({ user }) with no projection, so the device
list endpoint returned the full document, including the fcmToken push
credential and the hardware serial, straight to the browser. Neither is
used by the dashboard or the Android client.
The query now projects both fields out with '-fcmToken -serial'. Only the
user-facing list is narrowed; getDeviceById, which feeds the guards and
internal logic, is untouched.
Guarded by the existing getDevicesForUser test, rewritten to assert the
projection and seen to fail against the pre-change single-argument call.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
1 participant
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.
Summary
Adds test coverage for the untested, security-critical parts of the backend, then makes three small fixes that were each written test-first. The goal is to make it safe to add features quickly without regressing what works today. Test count 64 to 121.
Tests land first and lock current behaviour; the fixes follow, each guarded by a test that was seen to fail against the pre-change code and pass after.
Tests (behaviour locks, no code change)
AuthGuard,CanModifyDevice, andCanModifyApiKey.X-Signatureheader equals an independently computed HMAC of the payload; delivery is aborted when the subscription is inactive or deleted.Fixes (small, each guarded)
await, so the checks never took effect (register accepted a malformed email or out-of-range password; change-password accepted a too-short one). Addingawaitmakes the existing rules apply at their intended point. Legitimate clients are unaffected: the web forms already enforce these, so only input the service always meant to reject is now rejected, with a clean 400.RegExp; a prefix with regex metacharacters could fail the request or mis-match. It is now escaped. TheescapeRegExphelper (with its existing spec) moved tosrc/commonso auth and gateway share one copy.Deferred, with reasons
ValidationPipe.class-transformer(a required peer) is not installed, so enabling the pipe would add a dependency and risk breaking once any DTO gains a decorator. The validation win is delivered instead by fix V2 #1, which needs no new dependency. Recommended as a follow-up: add the dependency, enable the pipe, and decorate the auth DTOs, guarded by controller tests.Verification
Full suite green (121),
pnpm buildclean.🤖 Generated with Claude Code