Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }) | ||
| } | ||
|
|
||
| // TODO : Merge update and updateAnd into one function that takes an array of conditions as argument |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| send(res, 200, { valid: false }) | ||
| } else { | ||
| // TO DO : ensure that the pubkey only appears one time | ||
| // TODO : ensure that the pubkey only appears one time |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| @@ -0,0 +1,50 @@ | |||
| /* istanbul ignore file */ | |||
|
|
|||
| // TODO : Verify the content of this file while we implement the spec. There is absolutely no guarantee that this is correct. | |||
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
|
View your CI Pipeline Execution ↗ for commit e0ebd8c
☁️ Nx Cloud last updated this comment at |
…terns - Add regex.ts with validation functions for Matrix IDs, room IDs, emails, etc. - Add size_limits.md documenting Matrix protocol size constraints - Add eventTypes.ts with Matrix event type constants - Refactor validateParameters to support value validation callbacks - Add enhanced logging to send() function with status-based logging - Update hostname validation to use new isHostnameValid utility - Add extensive test coverage for regex validation functions - Export validation utilities from utils package index
- Add 'badJson' error type for JSON parsing errors - Replace generic 'unknown' error with more specific badJson error in jsonContent
…sability - Extract lookup logic into standalone lookup3pid function - Allow internal calls to lookup without HTTP layer - Improve error handling with proper error string conversion - Update hash_details to use new validation utilities
…er async handling - Export getUrlsFromPolicies for external use - Refactor fillPoliciesDB to return Promise and handle existing policies - Update existing policies instead of only inserting new ones - Add proper error handling and re-throwing in fillPoliciesDB - Update tests to handle async fillPoliciesDB function
… redundant HTTP calls - Replace HTTP lookup call with direct lookup3pid function call in invitation - Add phone number validation regex - Remove redundant fetch import from invitation handler - Update 3pid bind and validation endpoints to use new validation utilities - Update key management endpoints with improved error handling - Simplify matrix token validation logic
- Update test suites to work with async fillPoliciesDB - Export computePolicy and getUrlsFromPolicies for testing - Improve database test coverage - Update SQL schema and queries for better type safety
- Update authentication utilities to use new validation functions - Update tests for identity server caching - Align with matrix-identity-server refactoring changes
- Add DOCKER-VARIABLES.md with comprehensive docker configuration guide - Add HOW_TO_CONTINUE.md noting crypto implementation needs review - Update invitation.md with implementation details - Update various package READMEs - Update architecture diagram and OpenAPI spec
pm-McFly
force-pushed
the
full-id-service-revamped
branch
from
c1f0012 to
f27906b
Compare
WalkthroughThis pull request encompasses a substantial refactoring across infrastructure, CI/CD workflows, and the matrix identity server codebase. It adds LDAP Docker infrastructure with system dependencies, keyring configuration, and database initialization. GitHub Actions workflows are extended to support the dev branch and workflow\_call triggers across CodeQL, DevSkim, njsscan, and PR validation pipelines. The matrix identity server undergoes significant changes including error handling improvements (converting errors to strings), token management refactoring, hostname validation consolidation via shared utilities, and parameter validation centralization. Database schema updates introduce IF NOT EXISTS guards for table and index creation. Test suites transition from callback-based to async/await patterns. New utility modules provide regex-based validators for Matrix identifiers and hostname validation alongside access token extraction helpers. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tom-server/src/identity-server/with-cache.test.ts (1)
326-370: Clarify or remove commented-out test block.This substantial test block for account registration and logout has been commented out without explanation. Consider either:
- Removing it if the functionality is no longer needed
- Re-enabling it if the tests are still relevant
- Adding a TODO/FIXME comment explaining why it's disabled and when it should be re-enabled
packages/matrix-identity-server/src/3pid/index.ts (1)
16-17: Parameter validation allows empty strings.The condition
prms.client_secret?.length != null && prms.sid?.length != nullchecks whether thelengthproperty exists, not whether the strings are non-empty. Empty strings have alengthof 0, which passes this check.Apply this diff to properly validate non-empty strings:
- if (prms.client_secret?.length != null && prms.sid?.length != null) { + if (prms.client_secret?.length > 0 && prms.sid?.length > 0) {Alternatively, simplify using truthiness:
- if (prms.client_secret?.length != null && prms.sid?.length != null) { + if (prms.client_secret && prms.sid) {packages/matrix-identity-server/src/index.test.ts (1)
1795-1829: DuplicatebeforeAllhooks detected in the same describe block.The static analysis correctly identifies this as problematic. Having two
beforeAllhooks in the same describe block makes execution order unclear and can cause race conditions. Additionally,fillPoliciesDBreturns a Promise but is not awaited, so the test may run before policies are set up.Merge the hooks and await the async operation:
- beforeAll((done) => { - conf2 = { - ...defaultConfig, - database_engine: 'sqlite', - base_url: 'http://example.com/', - userdb_engine: 'sqlite', - policies - } - idServer2 = new IdServer(conf2) - app2 = express() - - idServer2.ready - .then(() => { - Object.keys(idServer2.api.get).forEach((k) => { - app2.get(k, idServer2.api.get[k]) - }) - Object.keys(idServer.api.post).forEach((k) => { - app2.post(k, idServer2.api.post[k]) - }) - done() - }) - .catch((e) => { - done(e) - }) - }) - beforeAll(async () => { + beforeAll(async () => { + conf2 = { + ...defaultConfig, + database_engine: 'sqlite', + base_url: 'http://example.com/', + userdb_engine: 'sqlite', + policies + } + idServer2 = new IdServer(conf2) + app2 = express() + + await idServer2.ready + Object.keys(idServer2.api.get).forEach((k) => { + app2.get(k, idServer2.api.get[k]) + }) + Object.keys(idServer.api.post).forEach((k) => { + app2.post(k, idServer2.api.post[k]) + }) + idServer2.logger.info('Calling register to obtain a valid token') // ... rest of registration logic ... - try { - fillPoliciesDB(userId, idServer2, 0) + await fillPoliciesDB(userId, idServer2, 0) idServer2.logger.info('Successfully added policies for the user') - } catch (e) { - idServer2.logger.error('Error while setting up policies for the user', e) - } })
♻️ Duplicate comments (3)
packages/matrix-identity-server/src/keyManagement/validPubkey.ts (1)
23-23: Pre-existing TODO already flagged.This TODO comment was already identified by static analysis in a previous review.
packages/utils/src/eventTypes.ts (1)
3-3: Track the TODO comment to ensure spec verification.The TODO indicates that content verification is pending during spec implementation. Consider creating a tracking issue to ensure this verification happens.
packages/matrix-identity-server/src/db/sql/sqlite.ts (1)
279-280: Track TODO: Function consolidation.The TODO references a completed implementation in the Client server (
updateWithConditions). Consider applying the same pattern here to reduce code duplication betweenupdateandupdateAnd.
🟡 Minor comments (8)
packages/matrix-identity-server/HOW_TO_CONTINUE.md-3-3 (1)
3-3: Fix typo and reduce wordiness.Line 3 contains a misspelling ("functionning" should be "functioning") and verbose phrasing. Simplify "in order to ensure" to "to ensure".
Apply this diff:
-The implementation is complete in terms of requirements from the spec. Nevertheless the key management and all the crypto interactions must be verified and corrected by someone with better skills and experience in order to ensure its good functionning. +The implementation is complete in terms of requirements from the spec. Nevertheless, the key management and all crypto interactions must be verified and corrected by an expert to ensure proper functioning.packages/logger/README.md-59-59 (1)
59-59: Fix spelling: "Aditionnal" → "Additional".Lines 59 and 328 use the misspelling "Aditionnal" instead of "Additional".
-Aditionnal details are displayed in the following order: +Additional details are displayed in the following order:-// Methods won't crash if they are called with unsupported additionnal detail +// Methods won't crash if they are called with unsupported additional detailAlso applies to: 328-328
packages/logger/README.md-46-46 (1)
46-46: Fix typo: "loggger" → "logger".Line 46 contains a spelling error that should be corrected.
-This loggger has a predefined format which is: +This logger has a predefined format which is:DOCKER-VARIABLES.md-1-3 (1)
1-3: Fix spelling errors in title and text.Line 1 contains "varibales" (should be "variables") and Line 3 contains "servce" (should be "service").
-# Docker varibales for ToM server +# Docker variables for ToM server- * `BASE_URL`: Base URL of the servce. Example: `https://tom.company.com` + * `BASE_URL`: Base URL of the service. Example: `https://tom.company.com`packages/matrix-identity-server/src/validate/email/requestToken.ts-32-32 (1)
32-32: Fix typo in constant name and reconsider the maximum value.The constant name has a typo:
maxAttempsshould bemaxAttempts. Additionally, the value of 1 billion seems extremely high for send attempts—consider whether a more reasonable limit (e.g., 100 or 1000) would be more appropriate for preventing abuse.Apply this diff to fix the typo:
-const maxAttemps = 1000000000 +const maxAttempts = 1000000000Don't forget to update the reference at line 167 as well.
Committable suggestion skipped: line range outside the PR's diff.
packages/utils/src/index.test.ts-548-551 (1)
548-551: Move misplaced test to correct describe block.This test validates
toMatrixIdlocalpart length limits but is placed inside theisValidUrldescribe block. It should be moved to thetoMatrixIddescribe block.Move this test to line 368 (inside the
toMatrixIddescribe block, after the existing length limit test):- describe('isValidUrl', () => { ...existing isValidUrl tests... - - it('should throw an error for a localpart longer than 512 characters', () => { - const longLocalpart = 'a'.repeat(513) - expect(() => toMatrixId(longLocalpart, 'example.com')).toThrowError() - }) - })Committable suggestion skipped: line range outside the PR's diff.
packages/utils/src/utils.ts-262-271 (1)
262-271:checkValuesmay passundefinedto validator functions.When a key exists in
valuesbut not inqueryParams, the validator receivesundefined. If validators don't handle this, they may throw or behave unexpectedly.Consider skipping undefined values or documenting the expected behavior:
export const checkValues = ( queryParams: Record<string, string>, values: queryParametersValueChecks ): void => { for (const key of Object.keys(values)) { + if (queryParams[key] === undefined) { + continue // Skip missing parameters + } if (!values[key](queryParams[key])) { throw new Error(`Invalid value for query parameter: ${key}`) } } }packages/utils/src/regex.ts-11-11 (1)
11-11: Email regex may reject valid addresses.The current pattern has several restrictions that could reject valid email addresses:
- Requires at least 2 characters before
@— single-char local parts likea@example.comare valid- TLD limited to 2-6 characters — TLDs like
.technology(10 chars) or.museum(6 chars) exist- Requires at least 2 characters after the last
.before@in the domainConsider a more permissive pattern or use a dedicated email validation library:
-const emailRegex: RegExp = /^\w[+.-\w]*\w@\w[.-\w]*\w\.\w{2,6}$/ +const emailRegex: RegExp = /^[^\s@]+@[^\s@]+\.[^\s@]{2,}$/Or for stricter validation while allowing single-char local parts:
-const emailRegex: RegExp = /^\w[+.-\w]*\w@\w[.-\w]*\w\.\w{2,6}$/ +const emailRegex: RegExp = /^[\w+.-]+@[\w.-]+\.\w{2,}$/
🧹 Nitpick comments (14)
packages/utils/src/size_limits.md (1)
15-15: Improve grammar flow: reposition "also" for clarity.Move "also" before the verb to improve sentence flow and readability.
Apply this diff to improve readability:
-Some event types have additional size restrictions which are specified in the description of the event. Additional restrictions exist also for specific room versions. Additional keys have no limit other than that implied by the total 64 KiB limit on events. +Some event types have additional size restrictions which are specified in the description of the event. Additional restrictions also exist for specific room versions. Additional keys have no limit other than that implied by the total 64 KiB limit on events.packages/matrix-identity-server/HOW_TO_CONTINUE.md (1)
1-4: Consider adding more specific guidance to this warning document.While the document serves as a helpful reminder about areas needing expert review, it lacks actionable details. Consider specifying:
- Which aspects of key management need verification (e.g., key generation, rotation, storage)?
- Which crypto interactions are of concern (e.g., signature validation, hashing)?
- Which cron tasks require verification and what should be checked?
- What expertise or role should handle this verification (e.g., cryptographer, DevOps)?
This would make the document more actionable for future maintainers.
DOCKER-VARIABLES.md (1)
3-38: Fix markdown list indentation.The unordered list items use inconsistent indentation. Top-level items should have 0 spaces (not 1), and nested items should have 2 spaces (not 3). This applies throughout the file.
-# Docker varibales for ToM server +# Docker variables for ToM server - * `BASE_URL`: Base URL of the servce. Example: `https://tom.company.com` - * `CRON_SERVICE`: Boolean (1 or 0): enable cron tasks or not - * `CROWDSEC_URI`: optional URI of local CrowdSec server - * `CROWDSEC_KEY`: CrowdSec API key _(required if `CROWDSEC_URI` is set) - * `DATABASE_ENGINE`: `sqlite` or `pg` - * `DATABASE_HOST`: - * case `pg`: hostname - * case `sqlite`: database path +* `BASE_URL`: Base URL of the service. Example: `https://tom.company.com` +* `CRON_SERVICE`: Boolean (1 or 0): enable cron tasks or not +* `CROWDSEC_URI`: optional URI of local CrowdSec server +* `CROWDSEC_KEY`: CrowdSec API key _(required if `CROWDSEC_URI` is set) +* `DATABASE_ENGINE`: `sqlite` or `pg` +* `DATABASE_HOST`: + * case `pg`: hostname + * case `sqlite`: database path -... (apply same fix to all remaining list items)packages/matrix-identity-server/src/3pid/bind.ts (5)
141-148: Error stringification looks good.The change correctly converts the error object to a string before passing it to
errMsg, which expects a string parameter.Optionally, consider using
err.messageinstead oferr.toString()for cleaner error messages (avoids "Error: " prefix):- send(res, 500, errMsg('unknown', err.toString())) + send(res, 500, errMsg('unknown', err.message))
150-154: Error stringification looks good.The change correctly converts the error object to a string before passing it to
errMsg.Optionally, consider using
err.messageinstead oferr.toString()for cleaner error messages:- send(res, 500, errMsg('unknown', err.toString())) + send(res, 500, errMsg('unknown', err.message))
156-160: Error stringification looks good.The change correctly converts the error object to a string before passing it to
errMsg.Optionally, consider using
err.messageinstead oferr.toString()for cleaner error messages:- send(res, 500, errMsg('unknown', err.toString())) + send(res, 500, errMsg('unknown', err.message))
187-192: Remove redundant console logging.The
console.infocall on line 189 duplicates theidServer.logger.infocall on line 188. The logger should be the single point of logging.if (!invitationTokens || !invitationTokens.length) { idServer.logger.info(`No pending invitations found for ${address}`) - console.info(`No pending invitations found for ${address}`) return }
217-220: Remove redundant console logging.The
console.errorcall on line 218 duplicates theidServer.logger.errorcall on line 219. The logger should be the single point of logging.} catch (error) { - console.error(`Failed to call onbind hook`, { error }) idServer.logger.error('Error calling onbind hook', error) }packages/matrix-identity-server/src/3pid/index.ts (1)
60-60: Error handling improved with explicit string conversion.Converting the error to a string with
err.toString()ensures type safety when passing toerrMsg, which expects an optional string parameter. This aligns with similar error-string handling patterns across the identity server codebase.Optional: Consider logging the full error (including stack trace) before sending the response to aid debugging:
.catch((err) => { /* istanbul ignore next */ + idServer.logger.error('Failed to retrieve validated 3pid:', err) send(res, 500, errMsg('unknown', err.toString())) })packages/matrix-identity-server/src/keyManagement/validEphemeralPubkey.ts (1)
23-23: Inconsistent TODO format.This uses
TO DO(with space) whilevalidPubkey.tsusesTODO(no space). The standard convention isTODOwithout a space for better tooling compatibility.- // TO DO : ensure that the pubkey only appears one time + // TODO: ensure that the pubkey only appears one timepackages/utils/src/regex.test.ts (1)
129-131: Consider renaming the test hostname for clarity.The test description says "should return true if the hostname is valid" but uses
'invalid.com'as the test value. While technically correct, consider using a clearer hostname like'example.com'to avoid confusion.- it('should return true if the hostname is valid', () => { - expect(isHostnameValid('invalid.com')).toBe(true) + it('should return true if the hostname is valid', () => { + expect(isHostnameValid('example.com')).toBe(true)packages/tom-server/src/identity-server/utils/authenticate.ts (1)
19-19: Remove unusedtokenReconstant on line 19.After refactoring to use
getAccessToken(req)on line 22, thetokenReregex constant is no longer referenced and can be safely removed to keep the code clean.packages/matrix-identity-server/src/db/index.ts (1)
657-657: Improve error message construction for token expiration.The error message concatenation could produce unclear output when
rows[0]is undefined, resulting in "Token expiredundefined". Consider a more explicit format:Apply this diff to improve the error message:
- 'Token expired' + (rows[0]?.expires as number)?.toString() + rows[0]?.expires != null + ? `Token expired at ${rows[0].expires}` + : 'Token not found or expired'packages/matrix-identity-server/src/invitation/index.ts (1)
138-138: Remove unused constant.The
validPhoneReregex pattern is defined but never referenced in the code. Phone validation usesvalidator.isMobilePhone(phone)instead (line 219).Apply this diff to remove the dead code:
-const validPhoneRe = /^\d{4,16}$/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/arch.dotis excluded by!**/*.dotpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (55)
.compose/ldap/Dockerfile(2 hunks).github/workflows/codeql.yml(2 hunks).github/workflows/devskim.yml(1 hunks).github/workflows/njsscan.yml(1 hunks).github/workflows/pr.yml(1 hunks).github/workflows/update-docs.yml(1 hunks)DOCKER-VARIABLES.md(1 hunks)README.md(0 hunks)docs/openapi.json(1 hunks)packages/federated-identity-service/Dockerfile(1 hunks)packages/federated-identity-service/package.json(1 hunks)packages/federated-identity-service/src/__testData__/ldap/Dockerfile(1 hunks)packages/federated-identity-service/templates/3pidInvitation.tpl(1 hunks)packages/logger/README.md(5 hunks)packages/matrix-identity-server/HOW_TO_CONTINUE.md(1 hunks)packages/matrix-identity-server/src/3pid/bind.ts(1 hunks)packages/matrix-identity-server/src/3pid/index.ts(1 hunks)packages/matrix-identity-server/src/additionalFeatures.test.ts(1 hunks)packages/matrix-identity-server/src/db/index.test.ts(5 hunks)packages/matrix-identity-server/src/db/index.ts(5 hunks)packages/matrix-identity-server/src/db/sql/_createTables.ts(4 hunks)packages/matrix-identity-server/src/db/sql/pg.ts(2 hunks)packages/matrix-identity-server/src/db/sql/sqlite.ts(2 hunks)packages/matrix-identity-server/src/index.test.ts(18 hunks)packages/matrix-identity-server/src/index.ts(3 hunks)packages/matrix-identity-server/src/invitation/index.ts(6 hunks)packages/matrix-identity-server/src/invitation/invitation.md(1 hunks)packages/matrix-identity-server/src/keyManagement/getPubkey.ts(2 hunks)packages/matrix-identity-server/src/keyManagement/validEphemeralPubkey.ts(3 hunks)packages/matrix-identity-server/src/keyManagement/validPubkey.ts(2 hunks)packages/matrix-identity-server/src/lookup/hash_details.ts(1 hunks)packages/matrix-identity-server/src/lookup/index.ts(2 hunks)packages/matrix-identity-server/src/terms.test.ts(1 hunks)packages/matrix-identity-server/src/terms/index.post.ts(4 hunks)packages/matrix-identity-server/src/utils.ts(3 hunks)packages/matrix-identity-server/src/utils/validateMatrixToken.ts(2 hunks)packages/matrix-identity-server/src/validate/email/requestToken.ts(7 hunks)packages/matrix-identity-server/src/validate/email/submitToken.ts(1 hunks)packages/matrix-identity-server/templates/3pidInvitation.tpl(2 hunks)packages/matrix-invite/Dockerfile(1 hunks)packages/matrix-invite/README.md(1 hunks)packages/matrix-invite/src/components/Confirmation.svelte(1 hunks)packages/retry-promise/README.md(2 hunks)packages/tom-server/src/identity-server/index.test.ts(2 hunks)packages/tom-server/src/identity-server/utils/authenticate.ts(2 hunks)packages/tom-server/src/identity-server/with-cache.test.ts(2 hunks)packages/tom-server/templates/3pidInvitation.tpl(1 hunks)packages/utils/src/errors.ts(3 hunks)packages/utils/src/eventTypes.ts(1 hunks)packages/utils/src/index.test.ts(6 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/regex.test.ts(1 hunks)packages/utils/src/regex.ts(1 hunks)packages/utils/src/size_limits.md(1 hunks)packages/utils/src/utils.ts(7 hunks)
💤 Files with no reviewable changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T16:08:30.680Z
Learnt from: pm-McFly
Repo: linagora/ToM-server PR: 302
File: packages/amqp-connector/package.json:26-33
Timestamp: 2025-12-16T16:08:30.680Z
Learning: To ensure consistency across the monorepo, advertise the Node.js version by adding an engines.node field to package.json in all packages (e.g., "engines": {"node": "18.20.8"}). This makes the runtime expectation explicit for reviewers and CI. Apply this across all packages in the repo, not just the one mentioned, and plan an upgrade in a future PR once the packages are stabilized and rationalized.
Applied to files:
packages/federated-identity-service/package.json
🧬 Code graph analysis (16)
packages/matrix-identity-server/src/lookup/index.ts (2)
packages/matrix-identity-server/src/index.ts (1)
MatrixIdentityServer(74-279)packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/db/sql/_createTables.ts (2)
packages/tom-server/src/utils.ts (1)
tables(1-15)packages/matrix-identity-server/src/index.ts (1)
logger(94-96)
packages/utils/src/regex.test.ts (1)
packages/utils/src/regex.ts (12)
isClientSecretValid(16-17)isEventTypeValid(19-20)isMatrixIdValid(22-23)isRoomIdValid(29-30)isSidValid(32-32)isStateKeyValid(34-35)isCountryValid(37-38)isPhoneNumberValid(40-41)isEmailValid(43-43)isRoomAliasValid(45-46)isHostnameValid(48-49)isSenderLocalpartValid(25-27)
packages/matrix-identity-server/src/keyManagement/getPubkey.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/tom-server/src/identity-server/utils/authenticate.ts (1)
packages/utils/src/utils.ts (1)
getAccessToken(539-556)
packages/matrix-identity-server/src/keyManagement/validEphemeralPubkey.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/index.ts (1)
packages/utils/src/regex.ts (1)
isHostnameValid(48-49)
packages/matrix-identity-server/src/3pid/index.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/terms/index.post.ts (3)
packages/matrix-identity-server/src/terms/index.ts (1)
Policies(11-14)packages/matrix-identity-server/src/types.ts (1)
DbGetResult(70-72)packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/lookup/hash_details.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/utils/src/index.test.ts (1)
packages/utils/src/utils.ts (6)
send(15-41)validateParameters(167-175)validateParametersAndValues(186-195)toMatrixId(427-455)isValidUrl(517-525)getAccessToken(539-556)
packages/matrix-identity-server/src/3pid/bind.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/validate/email/requestToken.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/validate/email/submitToken.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/utils/src/utils.ts (2)
packages/logger/src/index.ts (1)
TwakeLogger(11-11)packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/utils.ts (2)
packages/utils/src/utils.ts (1)
getAccessToken(539-556)packages/utils/src/errors.ts (1)
errMsg(144-153)
🪛 Biome (2.1.2)
packages/matrix-identity-server/src/index.test.ts
[error] 1795-1829: Disallow duplicate setup and teardown hooks.
Disallow beforeAll duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
packages/federated-identity-service/package.json
[error] 46-46: The key @twake/utils was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🪛 Checkov (3.2.334)
docs/openapi.json
[high] 1: Ensure that API keys are not sent over cleartext
(CKV_OPENAPI_20)
[medium] 1: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 GitHub Check: devskim
packages/matrix-identity-server/src/db/sql/sqlite.ts
[notice] 279-279: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
packages/matrix-identity-server/src/keyManagement/validPubkey.ts
[notice] 23-23: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
packages/utils/src/eventTypes.ts
[notice] 3-3: A "TODO" or similar was left in source code, possibly indicating incomplete functionality
Suspicious comment
🪛 LanguageTool
DOCKER-VARIABLES.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # Docker varibales for ToM server * BASE_URL: Base URL...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: Ensure spelling is correct
Context: ... server * BASE_URL: Base URL of the servce. Example: https://tom.company.com * ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
packages/matrix-identity-server/HOW_TO_CONTINUE.md
[style] ~3-~3: Consider a more concise word here.
Context: ...meone with better skills and experience in order to ensure its good functionning. The manag...
(IN_ORDER_TO_PREMIUM)
packages/utils/src/size_limits.md
[style] ~15-~15: To make your writing flow more naturally, try moving ‘also’ before the verb.
Context: ...n of the event. Additional restrictions exist also for specific room versions. Additional ...
(ALSO_PLACEMENT)
packages/logger/README.md
[grammar] ~46-~46: Ensure spelling is correct
Context: ...is named log_level #### Format This loggger has a predefined format which is: `LEVE...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
DOCKER-VARIABLES.md
3-3: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
4-4: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
5-5: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
6-6: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
23-23: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
24-24: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
27-27: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
30-30: Unordered list indentation
Expected: 2; Actual: 3
(MD007, ul-indent)
31-31: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
32-32: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
33-33: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build And Test / test
🔇 Additional comments (60)
docs/openapi.json (1)
1-1: Formatting-only change; no functional impact on OpenAPI specification.The AI summary indicates this is a whitespace/formatting-only change with no semantic differences. Since the OpenAPI spec defines only the stable API surface and no endpoints or schema definitions have been modified, this change is safe to proceed.
Note: Static analysis flags (
CKV_OPENAPI_20,CKV_OPENAPI_21) appear to be pre-existing concerns rather than issues introduced by this PR. These may be worth addressing in a separate effort if not already tracked.packages/matrix-invite/Dockerfile (1)
24-24: LGTM! Whitespace cleanup.The trailing space removal improves file formatting without any functional impact.
packages/federated-identity-service/src/__testData__/ldap/Dockerfile (1)
5-5: LGTM! Comment formatting cleanup.The trailing space removal from the comment improves consistency without any functional impact.
.compose/ldap/Dockerfile (1)
6-6: LGTM! Formatting improvements.The formatting adjustments to the comment and cleanup command improve consistency without affecting the Docker build process or runtime behavior.
Also applies to: 19-19
packages/federated-identity-service/package.json (1)
57-60: LGTM! Engines field properly configured.The Node.js and npm versions are explicitly declared in the engines field, making the runtime expectations clear for reviewers and CI.
Based on learnings, this configuration is consistent with the monorepo's standardization efforts.
packages/federated-identity-service/templates/3pidInvitation.tpl (1)
5-5: LGTM: Trailing whitespace removed from Subject header.The removal of trailing whitespace from the email Subject header is a good cleanup. This change is consistent with similar improvements across other 3PID invitation templates in this PR.
packages/tom-server/templates/3pidInvitation.tpl (1)
5-5: LGTM: Trailing whitespace cleanup.The trailing whitespace removal from the Subject header maintains consistency with the same cleanup applied across other invitation templates in this PR.
packages/matrix-identity-server/templates/3pidInvitation.tpl (2)
5-5: LGTM: Trailing whitespace removed.Consistent whitespace cleanup across all invitation templates.
15-16: LGTM: Improved plain text formatting.The plain text email body formatting has been adjusted to place the invitation link on a separate line, improving readability.
packages/matrix-identity-server/src/invitation/invitation.md (1)
4-19: LGTM: Documentation formatting improved.The schema presentation has been reformatted with horizontal rules and a clearer tabular layout, making the parameter requirements for different media types (email vs msisdn) easier to understand. No semantic changes to the specification.
packages/tom-server/src/identity-server/with-cache.test.ts (1)
153-157: LGTM! Mock now correctly simulates async behavior.The updated mock properly returns a Promise from
json(), which accurately reflects the actual behavior of the Fetch API. The eslint-disable comment is appropriate here since returningPromise.resolve()doesn't requireasyncsyntax.packages/federated-identity-service/Dockerfile (5)
59-59: LGTM!The CMD instruction is correct and uses the safer array syntax form. The change here is purely formatting with no functional impact.
5-26: Multi-stage build is well-structured.The builder stage correctly compiles and optimizes the application, with thorough cleanup of build artifacts before the production install. This follows Docker best practices for minimal final image size.
15-21: Package copying is complete.All required packages including the new/enhanced packages/utils are correctly copied for the refactored identity service infrastructure.
30-52: Environment variables configuration is appropriate.All necessary configuration variables are properly declared without hardcoded secrets, allowing flexibility for different deployment environments.
2-2: Node.js 18 has reached end-of-life (April 30, 2025); migration to Node.js 20 or 22 is already planned.The Dockerfile uses node:18.20.8-alpine as the base image. Node.js 18 is scheduled to reach end-of-life on 30 April 2025 and no longer receives security updates. Per the project learnings, this version is intentionally pinned temporarily for stability reasons, with an upgrade to a supported Node.js LTS version (20 or 22) planned for a future PR once packages are stable and rationalized. No action required for this PR.
Also applies to: 5-5, 28-28
packages/matrix-identity-server/src/validate/email/requestToken.ts (1)
84-93: Good refactor: Function name now accurately reflects behavior.Renaming from
fillTabletofillTableAndSendimproves code clarity by explicitly indicating that this function both stores data and sends an email.packages/tom-server/src/identity-server/index.test.ts (2)
195-199: LGTM! Mock now correctly returns Promise.Making
json()return a Promise aligns with the actualfetchAPI behavior and the PR's async refactoring. The ESLint directive is appropriate since the arrow function directly returns a Promise without needing theasynckeyword.
505-548: Clarify intent of commented-out account tests or remove them entirely.The account registration and logout tests (lines 505-548) are commented out with no explanation—no TODO, FIXME, or comment indicating whether they're temporarily disabled pending feature work or simply obsolete. Since
federatedIdentityTokenexists only to support these commented tests, either add a clear comment explaining when they'll be re-enabled, or remove the dead code to reduce maintenance burden..github/workflows/devskim.yml (1)
10-13: LGTM!The expanded push triggers and
workflow_calladdition enable this workflow to be invoked by the orchestrator workflow (pr.yml) while maintaining independent triggers on push to master/dev. The configuration is correct..github/workflows/codeql.yml (2)
16-19: LGTM!The expanded push triggers and
workflow_calladdition correctly enable this workflow to be reused by pr.yml while maintaining independent triggers on master/dev branches.
72-72: LGTM!Minor YAML formatting improvement removing trailing space after colon.
.github/workflows/njsscan.yml (1)
13-16: LGTM!The expanded push triggers and
workflow_calladdition enable this workflow to be invoked by pr.yml. The configuration aligns with the reusable workflow pattern used across other workflows in this PR..github/workflows/pr.yml (1)
13-24: LGTM!The new jobs correctly orchestrate the security analysis workflows (CodeQL, DevSkim, njsscan) using the reusable workflow pattern. Each job properly references its workflow file and inherits secrets. This setup centralizes PR analysis while allowing individual workflows to maintain their independent triggers.
.github/workflows/update-docs.yml (1)
5-11: Update branch configuration in update-docs.yml for consistency with other security workflows.The workflow uses
mainandmasterbranches, while devskim.yml, codeql.yml, and njsscan.yml all trigger onmasteranddev. Since other push-based security workflows in the repository targetmasteranddev, align update-docs.yml to use the same branches unless documentation updates require a different strategy.packages/matrix-identity-server/src/lookup/hash_details.ts (1)
18-21: LGTM! Consistent error stringification.Converting the error to string ensures type-safe usage of
errMsg()which expectsstring | undefinedfor the explanation parameter. This aligns with the broader error-handling standardization across the identity server.packages/matrix-identity-server/src/keyManagement/validPubkey.ts (2)
5-6: Good refactor: Direct DB dependency injection.Changing the parameter from
idServertoidServerDBimproves clarity and reduces coupling - this handler only needs database access, not the full identity server instance.
27-30: Consistent error handling.Error stringification matches the pattern established across the codebase.
packages/matrix-identity-server/src/utils.ts (3)
6-6: Good consolidation of token extraction logic.Importing
getAccessTokenfrom@twake/utilscentralizes Bearer token handling that was previously duplicated across modules.
56-64: Correct error status for database failures.Returning 500 for database errors is semantically appropriate - these are server-side failures, not authentication issues. The
/* istanbul ignore next */comments correctly mark these difficult-to-test error paths.
70-78: Consistent error handling for token lookup failures.Same pattern as above - database errors correctly return 500 instead of 401.
packages/matrix-identity-server/src/db/sql/_createTables.ts (2)
22-24: Good: Idempotent table creation.Using
CREATE TABLE IF NOT EXISTSmakes initialization safe for restarts and re-runs.
36-38: Good: Idempotent index creation.Using
CREATE INDEX IF NOT EXISTSprevents errors on repeated initialization.packages/matrix-identity-server/src/keyManagement/validEphemeralPubkey.ts (2)
5-6: Consistent with validPubkey.ts refactor.Same pattern of receiving the DB directly rather than the full server instance.
27-30: Consistent error handling.Error stringification matches the established pattern.
packages/matrix-identity-server/src/db/sql/pg.ts (1)
712-719: Breaking change:fieldsparameter is now required.The
fieldsparameter ingetMaxWhereEqualAndLoweris defined asfields: string[](required, without optional marker). All callers have been updated to pass the fields array; verify that external consumers of this API have similarly been updated.packages/utils/src/eventTypes.ts (1)
5-50: LGTM! Well-structured event type constants.The event type mapping is comprehensive and follows Matrix specification conventions. The use of
Setto derivevalidEventTypesensures uniqueness, which is a good defensive practice.packages/matrix-identity-server/src/utils/validateMatrixToken.ts (1)
3-19: LGTM! Good refactor to centralized validation.Replacing the inline regex validation with the
isHostnameValidutility improves code reuse and maintainability. The error handling behavior remains consistent.packages/tom-server/src/identity-server/utils/authenticate.ts (1)
4-4: LGTM! Good refactor to centralized token extraction.Using
getAccessToken(req)from the utils package improves code reuse and reduces duplication of token extraction logic.Also applies to: 22-22
packages/utils/src/index.ts (1)
4-5: LGTM! Proper API surface expansion.The new re-exports correctly expand the public API to include the event types and regex validation utilities, following standard module organization patterns.
packages/matrix-identity-server/src/terms.test.ts (1)
78-83: LGTM! Corrected mock to match actualfetchbehavior.The updated mock now properly returns a Promise from
json(), aligning with the actual behavior of thenode-fetchlibrary. This improves test accuracy.packages/matrix-identity-server/src/additionalFeatures.test.ts (1)
107-112: LGTM! Consistent mock improvement.The mock now correctly returns a Promise from
json(), matching the actualnode-fetchbehavior. This change is consistent with similar updates in other test files.packages/utils/src/regex.test.ts (1)
16-150: LGTM! Comprehensive test coverage for validation utilities.The test suite provides excellent coverage of all validation functions with positive cases, negative cases, and boundary conditions (e.g., length limits). This will help ensure the reliability of the validation logic across the codebase.
packages/matrix-identity-server/src/db/sql/sqlite.ts (1)
743-764: All callers ofgetMaxWhereEqualAndLowerprovide the requiredfieldsparameter. The parameter change from optional to required has been properly implemented across all usages in the codebase. No further action needed.packages/utils/src/errors.ts (1)
17-18: LGTM! New error codes align with validation improvements.The additions of
unknownParamandinvalidTokenerror codes are consistent with the existing error code structure and support the enhanced validation logic introduced in this PR.Also applies to: 60-61
packages/matrix-identity-server/src/keyManagement/getPubkey.ts (2)
18-30: LGTM! Fallback logic improves key retrieval robustness.The addition of a fallback to query
longTermKeypairswhenshortTermKeypairsdoesn't yield exactly one match is a good enhancement. This ensures the endpoint can still serve public keys from long-term storage when appropriate.
36-36: Improved error handling with explicit stringification.Converting the error to a string before sending ensures consistent error message formatting in 500 responses.
packages/matrix-identity-server/src/index.ts (1)
131-141: LGTM! Enhanced validation with better abstraction.The migration from direct regex usage to
isHostnameValidimproves code clarity and maintainability. The normalization logic correctly handles both array and comma/space-separated string forms offederated_identity_services.packages/matrix-identity-server/src/db/index.ts (1)
547-548: LGTM! Improved error propagation consistency.The explicit rejection in catch blocks ensures that errors are properly propagated to calling code, improving the reliability of error handling throughout the database operations.
Also applies to: 580-581, 635-636, 663-664, 734-735
packages/matrix-identity-server/src/lookup/index.ts (1)
16-35: LGTM! Good refactoring for code reuse.The extraction of
lookup3pidas a separate exported function aligns with the PR objective of improving code reuse. This function can now be used directly by other modules (like the invitation flow) without making redundant HTTP calls.packages/matrix-identity-server/src/invitation/index.ts (1)
243-266: LGTM! Excellent optimization replacing HTTP call with direct function invocation.The replacement of the HTTP-based lookup with a direct call to
lookup3pideliminates redundant network overhead and aligns with the PR's objective of reducing unnecessary HTTP calls in the invitation flow. The hashing logic correctly uses 'mail' as the field name for email medium, maintaining consistency with the lookup implementation.packages/matrix-identity-server/src/db/index.test.ts (1)
204-301: LGTM! Comprehensive tests for updateAnd functionality.The new test cases thoroughly validate the
updateAndmethod with multiple scenarios: matching conditions, non-matching conditions, and return value verification. Good coverage.packages/utils/src/index.test.ts (1)
55-69: LGTM! Comprehensive test coverage for new utilities.The expanded test coverage for logging behavior, parameter validation (including
validateParametersAndValues), andgetAccessTokenthoroughly exercises the new functionality introduced in this PR.Also applies to: 188-204, 207-271, 584-627
packages/matrix-identity-server/src/terms/index.post.ts (1)
40-92: LGTM on the async fillPoliciesDB refactor.The upsert pattern with
Promise.allis correctly implemented. The function properly checks for existing records before deciding to update or insert, and re-throws errors to allow callers to handle failures.packages/matrix-identity-server/src/index.test.ts (2)
562-589: Good addition of validation tests forsend_attempt.These tests ensure proper rejection of non-numeric and excessively large values for the
send_attemptparameter, improving input validation coverage.
1338-1365: Test setup correctly inserts and cleans up hash data.The test now properly sets up the hashes table with a computed hash before testing the
M_THREEPID_IN_USEscenario and cleans up afterward.packages/utils/src/regex.ts (1)
1-49: Good centralization of validation utilities.The regex validators provide a single source of truth for Matrix ID, room ID, client secret, and other format validations. The byte length checks correctly enforce the Matrix spec limit of 255 characters.
packages/utils/src/utils.ts (3)
539-556: Potential runtime error when accessingreq.queryonIncomingMessage.
http.IncomingMessagedoes not have aqueryproperty. While the@ts-expect-errorsuppresses TypeScript errors, this could fail at runtime if called with a rawIncomingMessage.Verify that
getAccessTokenis only called with ExpressRequestobjects, or add a fallback forIncomingMessage:export const getAccessToken = ( req: Request | http.IncomingMessage ): string | null => { const tokenRe = /^Bearer (\S+)$/ let token: string | null = null if (req.headers.authorization != null) { const re = req.headers.authorization.match(tokenRe) if (re != null) { token = re[1] } - // @ts-expect-error req.query exists - // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions - } else if (req.query && Object.keys(req.query).length > 0) { - // @ts-expect-error req.query.access_token may be null - token = req.query.access_token + } else if ('query' in req && req.query != null) { + const query = req.query as Record<string, unknown> + if (typeof query.access_token === 'string') { + token = query.access_token + } + } else if (req.url != null) { + // Fallback for IncomingMessage: parse query from URL + const url = new URL(req.url, 'http://localhost') + token = url.searchParams.get('access_token') } return token }
71-71: Good improvement: Using specificbadJsonerror code.Changing from generic
'unknown'to'badJson'provides clearer error semantics for JSON parsing failures.
15-41: Good addition of optional logging tosend().The conditional logging based on status code range is appropriate — debug for success, error for failures.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/matrix-identity-server/src/db/index.test.ts (2)
8-8: Prefer consistent assertion style.The import of
assertfrom 'console' is unusual for test files. The tests primarily useexpect(lines 401, 414 useassert). Consider replacingassert(tokens)calls withexpect(tokens).toBeDefined()for consistency with the rest of the test suite.
344-356: Consider reducing test timeout duration.The 6-second timeout makes this test relatively slow. If the timeout behavior can be validated with a shorter duration (e.g., 2-3 seconds), it would improve test suite execution time. However, if this duration is necessary for reliable validation of the expiration mechanism, the current implementation is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/matrix-identity-server/src/db/index.test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build And Test / test
🔇 Additional comments (1)
packages/matrix-identity-server/src/db/index.test.ts (1)
27-52: LGTM! Well-structured test helpers.The helper functions follow best practices by centralizing setup, teardown, and data seeding logic. The async/await patterns are correctly implemented, and the functions improve test readability and maintainability.
Fixed critical bug in buildCondition helper where joinFields[key] was incorrectly checked instead of filterFields[key]. This caused filter conditions to be silently ignored when joinFields was undefined or didn't contain the key, breaking non-join database queries. The fix aligns with the pattern used in _getMinMax method and the sqlite.ts implementation. Addresses: CodeRabbit review comment on PR #300
Replaced fire-and-forget forEach pattern with Promise.all() to ensure all database updates complete before sending the HTTP response. This prevents the server from responding with 200 OK before policy acceptance records are actually persisted to the database. Additionally, consolidated error handling to ensure a single 500 error response is sent if any update fails, rather than attempting multiple responses. Addresses: CodeRabbit review comment on PR #300
Added await keyword to fillPoliciesDB() calls in two beforeAll hooks (lines 1824 and 1859). The function returns a Promise but was being called without await, potentially causing tests to run before database setup completes. This fixes test flakiness and ensures proper test data initialization. Addresses: CodeRabbit review comment on PR #300
Removed err.toString() from error responses in requestToken.ts at four locations (lines 127, 134, 204, 225). Using toString() on error objects exposes internal implementation details like stack traces and database information to clients. Errors are still logged with full details for debugging purposes, but clients now receive only generic error messages, preventing information disclosure vulnerabilities. Addresses: CodeRabbit review comment on PR #300
pm-McFly
force-pushed
the
full-id-service-revamped
branch
from
851198b to
3918f79
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/matrix-identity-server/src/index.test.ts (1)
1795-1829: Merge duplicatebeforeAllhooks into a single block.Static analysis correctly flags this as a duplicate setup hook. Having two
beforeAllblocks in the same describe scope can lead to confusion about execution order and makes the test setup harder to follow.Consider merging with the first
beforeAllblock (lines 1770-1793):beforeAll((done) => { conf2 = { ...defaultConfig, database_engine: 'sqlite', base_url: 'http://example.com/', userdb_engine: 'sqlite', policies } idServer2 = new IdServer(conf2) app2 = express() idServer2.ready .then(() => { Object.keys(idServer2.api.get).forEach((k) => { app2.get(k, idServer2.api.get[k]) }) Object.keys(idServer.api.post).forEach((k) => { app2.post(k, idServer2.api.post[k]) }) - done() + // Registration and policy setup + idServer2.logger.info('Calling register to obtain a valid token') + const mockResponse = Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve({ + sub: '@dwho:example.com', + 'm.server': 'matrix.example.com:8448' + }) + }) + // @ts-expect-error mock is unknown + fetch.mockImplementation(async () => await mockResponse) + return request(app2) + .post('/_matrix/identity/v2/account/register') + .send({ + access_token: 'bar', + expires_in: 86400, + matrix_server_name: 'matrix.example.com', + token_type: 'Bearer' + }) + .set('Accept', 'application/json') + }) + .then((response) => { + expect(response.statusCode).toBe(200) + validToken2 = response.body.token + idServer2.logger.info('Adding the policies for the user in the db') + return fillPoliciesDB(userId, idServer2, 0) + }) + .then(() => { + idServer2.logger.info('Successfully added policies for the user') + done() }) .catch((e) => { + idServer2.logger.error('Error while setting up policies for the user', e) done(e) }) }) -beforeAll(async () => { - // ... second beforeAll content -})Alternatively, convert the entire setup to use
async/awaitfor cleaner code.
♻️ Duplicate comments (2)
packages/matrix-identity-server/src/validate/email/requestToken.ts (1)
165-169: Add lower bound validation forsend_attempt.The validation checks the upper bound but allows zero or negative values. Per Matrix specification conventions,
send_attemptshould be a positive integer (>= 1).} else if ( typeof sendAttempt !== 'number' || + sendAttempt < 1 || - sendAttempt > maxAttemps + sendAttempt > maxAttempts ) { send(res, 400, errMsg('invalidParam', 'Invalid send attempt'))packages/matrix-identity-server/src/terms/index.post.ts (1)
126-144: Race condition resolved.The previous race condition has been fixed. The code now properly awaits all database updates using
Promise.allbefore sending the success response, and error handling is consolidated to prevent double-send scenarios.
🧹 Nitpick comments (1)
packages/matrix-identity-server/src/terms/index.post.ts (1)
35-38: Clarify the documentation.The comment states "completely overwrites the user's previous policy acceptance status," but the implementation checks if each policy exists and either updates or inserts it. Consider rephrasing to: "Sets the acceptance status for all policies for a given user. Updates existing policy entries or creates new ones as needed."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/federated-identity-service/package.json(1 hunks)packages/matrix-identity-server/src/db/index.test.ts(4 hunks)packages/matrix-identity-server/src/db/sql/pg.ts(1 hunks)packages/matrix-identity-server/src/index.test.ts(18 hunks)packages/matrix-identity-server/src/terms/index.post.ts(4 hunks)packages/matrix-identity-server/src/validate/email/requestToken.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/federated-identity-service/package.json
- packages/matrix-identity-server/src/db/index.test.ts
- packages/matrix-identity-server/src/db/sql/pg.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/matrix-identity-server/src/validate/email/requestToken.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/terms/index.post.ts (3)
packages/matrix-identity-server/src/index.ts (1)
getUrlsFromPolicies(59-59)packages/matrix-identity-server/src/terms/index.ts (1)
Policies(11-14)packages/matrix-identity-server/src/types.ts (1)
DbGetResult(70-72)
packages/matrix-identity-server/src/index.test.ts (3)
server.mjs (2)
idServer(276-283)app(204-204)packages/crypto/src/index.ts (1)
Hash(8-42)packages/matrix-identity-server/src/terms/index.post.ts (1)
fillPoliciesDB(41-92)
🪛 Biome (2.1.2)
packages/matrix-identity-server/src/index.test.ts
[error] 1795-1829: Disallow duplicate setup and teardown hooks.
Disallow beforeAll duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build And Test / test
🔇 Additional comments (7)
packages/matrix-identity-server/src/validate/email/requestToken.ts (1)
123-135: Good improvement: Error messages no longer leak internal details.The change from
errMsg('unknown', err.toString())toerrMsg('unknown')prevents exposing internal implementation details to clients while still logging the full error for debugging. This addresses security concerns about information disclosure.packages/matrix-identity-server/src/index.test.ts (4)
562-589: Good test coverage forsend_attemptvalidation.These tests properly verify the new validation logic for non-numeric and excessively large
send_attemptvalues, ensuring appropriate 400 responses and no email being sent.
1338-1365: Proper test data setup and cleanup for hash lookup.The test now correctly sets up hashed address data in the database and cleans up after the assertion, ensuring test isolation.
185-190: Mock responses updated to use Promise-basedjson()returns.Converting from synchronous to async
json()returns aligns with the actualnode-fetchAPI behavior, making tests more realistic.
133-135: New test validates federated identity services filtering.This test verifies that invalid federated identity services are properly filtered to an empty array during configuration.
packages/matrix-identity-server/src/terms/index.post.ts (2)
16-33: LGTM! Export aligns with PR objectives.Exposing
getUrlsFromPoliciesfor reuse aligns with the PR's goal of improved code reusability and centralized validation logic.
105-105: No action required. The userId derivation fromdata.subis correct.Using
data.subto obtain userId is the established and correct pattern throughout the codebase (as seen in account/index.ts), properly leveraging the JWT subject claim for user identification. Theidparameter correctly represents the access token identifier, not the user ID.
packages/matrix-identity-server/src/validate/email/requestToken.ts
Outdated
Show resolved
Hide resolved
Fixed critical bug in extractQueryParameters where instanceof Request
check always failed because TypeScript interfaces don't exist at runtime.
Replaced with property-based check ('query' in req) which correctly
identifies Request objects.
This ensures query parameters are properly extracted from Request objects
instead of incorrectly falling through to URL parsing.
Addresses: CodeRabbit review comment on PR #300
Fixed two critical issues in Confirmation.svelte:
1. Uninitialized existingDomains variable that would cause runtime errors
2. Memory leak from store subscription not being cleaned up on component destroy
Solution: Use Svelte's automatic subscription syntax (\)
which handles both initialization and cleanup automatically.
Before:
let existingDomains: string[] // undefined!
preferredDomains.subscribe(domains => { existingDomains = domains })
// No cleanup -> memory leak
After:
$: existingDomains = // Auto-initialized and auto-cleaned up
Addresses: CodeRabbit review comments on PR #300
Removed exact duplicate test blocks across two test files: - packages/utils/src/index.test.ts: Removed duplicate isValidUrl describe block (lines 553-582) - packages/matrix-identity-server/src/db/index.test.ts: Removed duplicate test (lines 601-617) These duplicates were creating confusion and unnecessary test execution time. The original test blocks remain with full coverage. Addresses: CodeRabbit review comments on PR #300
Removed e.toString() from error responses in submitToken.ts at two locations (lines 76 and 83). Using toString() on error objects exposes internal implementation details like stack traces and database information to clients. Errors are still logged with full details server-side for debugging, but clients now receive only generic error messages, preventing information disclosure vulnerabilities. This is consistent with the fix applied to requestToken.ts in commit 962cf470. Addresses: CodeRabbit review comment on PR #300
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/matrix-identity-server/src/db/index.test.ts (1)
56-58: PostgreSQL test isolation issue.The
afterEachhook only deletes the SQLite database file but doesn't clean up PostgreSQL tables whenTEST_PG=yes. This can lead to test pollution where data from previous tests affects subsequent tests, causing flaky or incorrect test results.Apply this diff to add proper PostgreSQL cleanup:
+ afterEach(async () => { - afterEach(() => { - process.env.TEST_PG === 'yes' || fs.unlinkSync('./testdb.db') + if (process.env.TEST_PG === 'yes') { + // Clean up PostgreSQL tables + if (idDb) { + await idDb.deleteWhere('accessTokens', { field: 'id', operator: 'IS NOT', value: 'NULL' }) + await idDb.deleteWhere('oneTimeTokens', { field: 'id', operator: 'IS NOT', value: 'NULL' }) + await idDb.deleteWhere('invitationTokens', { field: 'id', operator: 'IS NOT', value: 'NULL' }) + await idDb.deleteWhere('userPolicies', { field: 'user_id', operator: 'IS NOT', value: 'NULL' }) + await idDb.deleteWhere('attempts', { field: 'email', operator: 'IS NOT', value: 'NULL' }) + await idDb.deleteWhere('shortTermKeypairs', { field: 'keyID', operator: 'IS NOT', value: 'NULL' }) + } + } else { + fs.unlinkSync('./testdb.db') + } })
🧹 Nitpick comments (4)
packages/matrix-identity-server/src/validate/email/requestToken.ts (2)
34-37: Strengthen type safety for the error field.The
errorfield is typed asany, which bypasses type checking. Based on theerrMsgfunction signature inpackages/utils/src/errors.ts, it returns an object witherrcodeanderrorproperties. Consider using a more specific type.Apply this diff to improve type safety:
interface ValidationError { statusCode: number - error: any + error: { errcode: string; error: string } }
196-264: Refactoring improves maintainability; consider async/await for future complexity reduction.The refactored validation logic and consistent error handling improve code quality. However, the callback-based structure still contributes to the cyclomatic complexity flagged by CodeScene (9 → 11).
For future consideration, refactoring to async/await would flatten the nesting and further improve readability:
- Replace callback chains with async/await
- Use try/catch for error handling
- Reduce indentation levels
The current implementation is correct and functional, so this is an optional enhancement for future iterations.
packages/matrix-identity-server/src/db/index.test.ts (2)
344-356: Consider reducing test timeout for faster execution.This test waits 6 seconds to verify token expiration, which significantly slows down the test suite. While the logic is correct, consider reducing the delays for faster feedback during development.
Apply this diff to reduce test execution time:
- const idDb = await setupIdDb({ database_vacuum_delay: 3 }) + const idDb = await setupIdDb({ database_vacuum_delay: 1 }) - const token = await idDb.createOneTimeToken({ a: 1 }, 1) + const token = await idDb.createOneTimeToken({ a: 1 }, 0.5) await new Promise<void>((resolve) => { setTimeout(async () => { await expect(idDb.verifyOneTimeToken(token)).rejects.toThrow() cleanupIdDb(idDb) resolve() - }, 6000) + }, 3000) })
461-478: Consider using helper function for consistency.For consistency with other tests, consider using the
insertAccessTokenshelper instead of manual inserts.Apply this diff:
it('should sort entry by order', async () => { const idDb = await setupIdDb() - await idDb.insert('accessTokens', { id: '2', data: '{}' }) - await idDb.insert('accessTokens', { id: '1', data: '{}' }) + await insertAccessTokens(idDb, [ + { id: '2', data: '{}' }, + { id: '1', data: '{}' } + ]) const rows = await idDb.get(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/matrix-identity-server/src/db/index.test.ts(4 hunks)packages/matrix-identity-server/src/validate/email/requestToken.ts(4 hunks)packages/matrix-identity-server/src/validate/email/submitToken.ts(1 hunks)packages/matrix-invite/src/components/Confirmation.svelte(1 hunks)packages/utils/src/index.test.ts(7 hunks)packages/utils/src/utils.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/matrix-invite/src/components/Confirmation.svelte
- packages/matrix-identity-server/src/validate/email/submitToken.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/matrix-identity-server/src/validate/email/requestToken.ts (2)
packages/utils/src/errors.ts (1)
errMsg(144-153)packages/utils/src/utils.ts (1)
isValidUrl(528-536)
packages/matrix-identity-server/src/db/index.test.ts (3)
packages/matrix-identity-server/src/types.ts (2)
Config(6-68)DbGetResult(70-72)packages/matrix-identity-server/src/index.ts (1)
logger(94-96)packages/crypto/src/index.ts (1)
randomString(48-54)
packages/utils/src/index.test.ts (3)
docs/swagger-ui-es-bundle-core.js (1)
it(2-2)packages/utils/src/utils.ts (5)
send(15-41)validateParameters(167-175)validateParametersAndValues(186-195)toMatrixId(438-466)getAccessToken(550-567)packages/matrix-identity-server/src/utils/sms-service.ts (1)
send(35-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build And Test / test
🔇 Additional comments (17)
packages/matrix-identity-server/src/validate/email/requestToken.ts (2)
39-75: Excellent refactoring with all past issues addressed.The validation logic is now centralized and all previous review concerns have been resolved:
- The typo
maxAttempshas been corrected tomaxAttempts(line 32)- Lower bound validation for
send_attemptis properly implemented (line 65)- Clean separation of validation concerns improves maintainability
127-179: Security improvement: error details no longer leaked to clients.The error handling has been improved to avoid exposing internal implementation details. Errors are still logged for debugging (lines 168, 175) while clients receive generic error messages. This addresses the security concern raised in previous reviews.
packages/matrix-identity-server/src/db/index.test.ts (3)
27-52: LGTM! Well-structured test helpers.The helper functions effectively reduce code duplication and improve test maintainability. The async/await patterns are correctly implemented throughout.
67-418: Excellent async/await conversion.The tests in this section demonstrate proper async/await patterns, consistent resource management, and good test structure. The refactoring significantly improves readability compared to callback-based tests.
420-690: Comprehensive SQL operation test coverage.This test suite thoroughly covers various SQL operations including joins, max/min queries, and complex conditions. The async/await conversion is properly implemented, and all tests include appropriate cleanup.
packages/utils/src/index.test.ts (5)
17-32: LGTM! Good test helpers.These helper functions effectively reduce code duplication and improve test readability.
48-50: LGTM!The additional mock logger methods support testing the new logging behavior in the
sendfunction.
72-86: LGTM!These tests properly verify the conditional logging behavior for 2xx (debug) and non-2xx (error) status codes.
219-273: LGTM! Comprehensive test coverage.The test suite thoroughly covers the success path, missing parameter validation, and invalid value validation for
validateParametersAndValues.
555-598: LGTM! Complete test coverage.The test suite thoroughly covers all branches of
getAccessToken: Authorization header extraction, fallback to query parameters, and null returns when no token is present.packages/utils/src/utils.ts (7)
18-30: LGTM! Good observability improvement.The optional logger parameter with conditional logging (debug for 2xx, error for non-2xx) enhances observability without breaking existing callers.
71-71: LGTM! More specific error type.Changing from
'unknown'to'badJson'provides clearer error semantics for JSON parsing failures.
96-195: LGTM! Well-structured refactoring.The extraction of
_validateParametersas a shared implementation with public wrappersvalidateParametersandvalidateParametersAndValuesfollows the DRY principle effectively. The value validation logic is cleanly integrated with appropriate error handling.Note: The code duplication warning from static analysis tools is expected here, as the functions intentionally share implementation through
_validateParameters.
198-221: LGTM! Properly handles runtime type detection.The property-based check (
'query' in req) correctly addresses the previous critical issue withinstanceof Request, which doesn't work at runtime since TypeScript interfaces don't exist after compilation. The implementation now properly detects Express requests vs. plain IncomingMessage objects.
224-282: LGTM! Clear query parameter validation utilities.The type and value validation functions provide a clean API for validating query parameters. The cyclomatic complexity warning on
checkTypesis acceptable given the straightforward type-checking logic.
285-296: LGTM!The
setDefaultValuesfunction provides a clean way to apply defaults to query parameters with proper type conversion.
550-567: The null-safety check is adequate due to short-circuit evaluation.The condition
req.query && Object.keys(req.query).length > 0safely prevents errors—ifreq.queryis null or undefined, the expression short-circuits before callingObject.keys(). However, for consistency with line 204 in the same file, consider using the more explicit pattern:'query' in req && typeof req.query === 'object' && req.query !== nullbefore accessingreq.query.access_token.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/utils/src/index.test.ts (1)
219-273: Good test coverage for the new validation utility.The tests correctly verify the three main code paths: successful validation, missing parameters, and invalid values. The use of helper functions maintains consistency with other test suites.
Consider adding a test for extra/additional parameters (similar to lines 200-216 in
validateParameters) to ensure complete explicit coverage, even though both functions delegate to the same underlying implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utils/src/index.test.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/utils/src/index.test.ts (1)
packages/utils/src/utils.ts (4)
send(15-41)validateParameters(167-175)validateParametersAndValues(186-195)getAccessToken(550-567)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build And Test / test
🔇 Additional comments (5)
packages/utils/src/index.test.ts (5)
8-9: LGTM!The new imports for
validateParametersAndValuesandgetAccessTokenare correctly added to support the expanded test coverage below.
17-32: LGTM!The test helper functions improve code reuse and readability. The
expectErrorResponsehelper appropriately verifies the complete error response flow, whileexpectNoResponsefocuses on the key indicator (nowriteHeadcall).
48-50: LGTM!Extending the mock logger with
log,info, anddebugmethods is necessary to support the enhanced logging tests added below.
72-86: LGTM!The tests correctly verify the conditional logging behavior of the
sendfunction:debugfor 2xx status codes anderrorfor non-2xx.
180-180: LGTM!The test updates correctly use the new helper functions and add valuable coverage for the additional parameters warning scenario.
Also applies to: 197-198, 200-216
- Update CodeQL, DevSkim, and njsscan workflow actions - Update PR workflow with latest action versions - Update update-docs workflow configuration
- Update base image versions in Dockerfiles - Update Confirmation component in matrix-invite - Add federated-identity-service package dependency
- Update template formatting and content across all services - Ensure consistent invitation email format
- Update dependency tree after package changes - Resolve dependency conflicts and updates
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…plication Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
Fixed critical bug in buildCondition helper where joinFields[key] was incorrectly checked instead of filterFields[key]. This caused filter conditions to be silently ignored when joinFields was undefined or didn't contain the key, breaking non-join database queries. The fix aligns with the pattern used in _getMinMax method and the sqlite.ts implementation. Addresses: CodeRabbit review comment on PR #300
Replaced fire-and-forget forEach pattern with Promise.all() to ensure all database updates complete before sending the HTTP response. This prevents the server from responding with 200 OK before policy acceptance records are actually persisted to the database. Additionally, consolidated error handling to ensure a single 500 error response is sent if any update fails, rather than attempting multiple responses. Addresses: CodeRabbit review comment on PR #300
Added await keyword to fillPoliciesDB() calls in two beforeAll hooks (lines 1824 and 1859). The function returns a Promise but was being called without await, potentially causing tests to run before database setup completes. This fixes test flakiness and ensures proper test data initialization. Addresses: CodeRabbit review comment on PR #300
Removed err.toString() from error responses in requestToken.ts at four locations (lines 127, 134, 204, 225). Using toString() on error objects exposes internal implementation details like stack traces and database information to clients. Errors are still logged with full details for debugging purposes, but clients now receive only generic error messages, preventing information disclosure vulnerabilities. Addresses: CodeRabbit review comment on PR #300
Fixed critical bug in extractQueryParameters where instanceof Request
check always failed because TypeScript interfaces don't exist at runtime.
Replaced with property-based check ('query' in req) which correctly
identifies Request objects.
This ensures query parameters are properly extracted from Request objects
instead of incorrectly falling through to URL parsing.
Addresses: CodeRabbit review comment on PR #300
Fixed two critical issues in Confirmation.svelte:
1. Uninitialized existingDomains variable that would cause runtime errors
2. Memory leak from store subscription not being cleaned up on component destroy
Solution: Use Svelte's automatic subscription syntax (\)
which handles both initialization and cleanup automatically.
Before:
let existingDomains: string[] // undefined!
preferredDomains.subscribe(domains => { existingDomains = domains })
// No cleanup -> memory leak
After:
$: existingDomains = // Auto-initialized and auto-cleaned up
Addresses: CodeRabbit review comments on PR #300
Removed exact duplicate test blocks across two test files: - packages/utils/src/index.test.ts: Removed duplicate isValidUrl describe block (lines 553-582) - packages/matrix-identity-server/src/db/index.test.ts: Removed duplicate test (lines 601-617) These duplicates were creating confusion and unnecessary test execution time. The original test blocks remain with full coverage. Addresses: CodeRabbit review comments on PR #300
Removed e.toString() from error responses in submitToken.ts at two locations (lines 76 and 83). Using toString() on error objects exposes internal implementation details like stack traces and database information to clients. Errors are still logged with full details server-side for debugging, but clients now receive only generic error messages, preventing information disclosure vulnerabilities. This is consistent with the fix applied to requestToken.ts in commit 962cf470. Addresses: CodeRabbit review comment on PR #300
…maxAttempts typo - Add validation to reject sendAttempt values < 1 - Fix typo: maxAttemps → maxAttempts - Prevents zero or negative sendAttempt values from bypassing validation Addresses GitHub Advanced Security and CodeRabbit review comments
…lexity - Extract validateNumberParam helper function - Extract validateBooleanParam helper function - Reduce cyclomatic complexity from 9 to simpler delegating function - Improve code maintainability and testability Addresses CodeScene code quality review comment
- Add expectErrorResponse helper for common error assertion pattern - Add expectNoResponse helper for success cases - Replace 9 instances of duplicated assertion code with helper calls - Improves test maintainability and readability Addresses CodeScene code quality review comment
- Replace $: reactive statement with \ (Svelte 5 runes mode) - Maintains memory leak fix and proper initialization - Fixes build error in Svelte 5 runes mode Follow-up to commit 2943384d
- Extract validateRequestTokenArgs function to reduce cyclomatic complexity - Replace nested if-else chain with early return pattern - Separate validation concerns from business logic - Reduce RequestToken function complexity from 11 to manageable level Addresses CodeScene code quality review comment
Signed-off-by: Pierre 'McFly' Marty <pmarty@linagora.com>
pm-McFly
force-pushed
the
full-id-service-revamped
branch
from
9c2e5aa to
06c6bf3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
(6 files improve in Code Health)
Gates Failed
(5 files with Complex Method, Code Duplication, Complex Conditional, Primitive Obsession, Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| index.test.ts | 1 advisory rule | 10.00 → 9.10 | Suppress |
| requestToken.ts | 2 advisory rules | 9.24 → 8.97 | Suppress |
| utils.ts | 2 advisory rules | 7.73 → 7.50 | Suppress |
| index.ts | 1 advisory rule | 8.95 → 8.93 | Suppress |
| _createTables.ts | 1 advisory rule | 9.31 → 9.30 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| index.post.ts | 9.59 → 10.00 | Complex Method |
| index.ts | 8.47 → 8.61 | Complex Method |
| utils.ts | 8.86 → 8.96 | Complex Method, Bumpy Road Ahead |
| additionalFeatures.test.ts | 9.39 → 10.00 | Code Duplication |
| index.test.ts | 7.18 → 8.82 | Lines of Code in a Single File, Code Duplication, Complex Method, Overall Code Complexity |
| authenticate.ts | 9.46 → 10.00 | Complex Method, Bumpy Road Ahead |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.compose/ldap/Dockerfile (1)
33-33: Critical: Missing leading slash in path.The
slapd-clicommand path is missing the leading/, which will cause the Docker build to fail when attempting to execute this command.Apply this diff to fix the path:
- usr/local/openldap/sbin/slapd-cli restoreconfig -b /var/backups/openldap/config-20230322180123.ldif && \ + /usr/local/openldap/sbin/slapd-cli restoreconfig -b /var/backups/openldap/config-20230322180123.ldif && \packages/matrix-identity-server/src/validate/email/submitToken.ts (1)
89-96: Inconsistent error handling exposes token verification errors to client.While lines 71-83 correctly use generic error messages, line 94 still concatenates the error object
(e as string)into the client-facing response. This inconsistency may expose internal error details such as database connection errors or stack traces.Apply this diff to align with the security pattern used elsewhere in this file:
.catch((e) => { idServer.logger.error('Token error', e) send( res, 400, - errMsg('invalidParam', 'Unknown or expired token' + (e as string)) + errMsg('invalidParam', 'Unknown or expired token') ) })packages/matrix-identity-server/src/index.test.ts (1)
1770-1829: Merge duplicatebeforeAllhooks into a single hook.Static analysis correctly identified that there are two
beforeAllhooks in the samedescribeblock (lines 1770 and 1795). This violates Jest best practices and can lead to unpredictable execution order.Merge the two hooks:
beforeAll((done) => { conf2 = { ...defaultConfig, database_engine: 'sqlite', base_url: 'http://example.com/', userdb_engine: 'sqlite', policies } idServer2 = new IdServer(conf2) app2 = express() idServer2.ready .then(() => { Object.keys(idServer2.api.get).forEach((k) => { app2.get(k, idServer2.api.get[k]) }) Object.keys(idServer.api.post).forEach((k) => { app2.post(k, idServer2.api.post[k]) }) - done() + // Continue with token registration + idServer2.logger.info('Calling register to obtain a valid token') + const mockResponse = Promise.resolve({ + ok: true, + status: 200, + json: () => + Promise.resolve({ + sub: '@dwho:example.com', + 'm.server': 'matrix.example.com:8448' + }) + }) + // @ts-expect-error mock is unknown + fetch.mockImplementation(async () => await mockResponse) + return mockResponse + }) + .then(() => { + return request(app2) + .post('/_matrix/identity/v2/account/register') + .send({ + access_token: 'bar', + expires_in: 86400, + matrix_server_name: 'matrix.example.com', + token_type: 'Bearer' + }) + .set('Accept', 'application/json') + }) + .then((response) => { + expect(response.statusCode).toBe(200) + validToken2 = response.body.token + idServer2.logger.info('Adding the policies for the user in the db') + return fillPoliciesDB(userId, idServer2, 0) + }) + .then(() => { + idServer2.logger.info('Successfully added policies for the user') + done() }) .catch((e) => { + idServer2.logger.error('Error while setting up policies for the user', e) done(e) }) }) - beforeAll(async () => { - idServer2.logger.info('Calling register to obtain a valid token') - const mockResponse = Promise.resolve({ - ok: true, - status: 200, - // eslint-disable-next-line @typescript-eslint/promise-function-async - json: () => - Promise.resolve({ - sub: '@dwho:example.com', - 'm.server': 'matrix.example.com:8448' - }) - }) - // @ts-expect-error mock is unknown - fetch.mockImplementation(async () => await mockResponse) - await mockResponse - const response = await request(app2) - .post('/_matrix/identity/v2/account/register') - .send({ - access_token: 'bar', - expires_in: 86400, - matrix_server_name: 'matrix.example.com', - token_type: 'Bearer' - }) - .set('Accept', 'application/json') - expect(response.statusCode).toBe(200) - validToken2 = response.body.token - - idServer2.logger.info('Adding the policies for the user in the db') - try { - await fillPoliciesDB(userId, idServer2, 0) - idServer2.logger.info('Successfully added policies for the user') - } catch (e) { - idServer2.logger.error('Error while setting up policies for the user', e) - } - })
♻️ Duplicate comments (1)
packages/utils/src/index.test.ts (1)
563-593: Remove duplicate test and add precedence test.Lines 563-571 and 585-593 are functionally identical—both test the case where headers and query are empty. Additionally, there's no test verifying that the Authorization header takes precedence over the query parameter when both are present.
Apply this diff to remove the duplicate and add the missing precedence test:
it('should return null if there is no authorization header', () => { const req = { headers: {}, query: {} } as unknown as Request const token = getAccessToken(req) expect(token).toBeNull() }) it('should return the access token from the query parameters', () => { const req = { headers: {}, query: { access_token: 'some-token' } } as unknown as Request const token = getAccessToken(req) expect(token).toBe('some-token') }) - it('should return null if there is no token in headers or query', () => { - const req = { - headers: {}, - query: {} - } as unknown as Request - - const token = getAccessToken(req) - expect(token).toBeNull() - }) + it('should prioritize Authorization header over query parameter', () => { + const req = { + headers: { + authorization: 'Bearer header-token' + }, + query: { + access_token: 'query-token' + } + } as unknown as Request + + const token = getAccessToken(req) + expect(token).toBe('header-token') + })
🧹 Nitpick comments (2)
packages/matrix-identity-server/src/validate/email/submitToken.ts (1)
71-83: Previous security concern addressed; consider testing these error paths.The error handlers now correctly send generic error messages without exposing internal details, which addresses the previous review feedback. However, the
// istanbul ignore nextcomments suggest these error paths are excluded from code coverage. Since these are legitimate error scenarios (database operation failures), they should ideally be tested with appropriate mocks or error injection.Consider removing the
// istanbul ignore nextcomments and adding test cases that simulate database failures to ensure these error paths work as expected:.catch((e) => { - // istanbul ignore next idServer.logger.error('Error while updating token', e) - // istanbul ignore next send(res, 500, errMsg('unknown')) }) }) .catch((e) => { - // istanbul ignore next idServer.logger.error('Error while deleting token', e) - // istanbul ignore next send(res, 500, errMsg('unknown')) })packages/matrix-identity-server/src/validate/email/requestToken.ts (1)
34-37: Consider using a more specific type for the error property.The
errorproperty is typed asany, buterrMsgalways returns anobject. Using a more specific type improves type safety.interface ValidationError { statusCode: number - error: any + error: object }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.compose/ldap/Dockerfile(2 hunks).github/workflows/codeql.yml(2 hunks).github/workflows/devskim.yml(1 hunks).github/workflows/njsscan.yml(1 hunks).github/workflows/pr.yml(1 hunks).github/workflows/update-docs.yml(1 hunks)packages/federated-identity-service/Dockerfile(1 hunks)packages/federated-identity-service/package.json(1 hunks)packages/federated-identity-service/src/__testData__/ldap/Dockerfile(1 hunks)packages/federated-identity-service/templates/3pidInvitation.tpl(1 hunks)packages/matrix-identity-server/src/db/index.test.ts(4 hunks)packages/matrix-identity-server/src/db/sql/pg.ts(1 hunks)packages/matrix-identity-server/src/index.test.ts(18 hunks)packages/matrix-identity-server/src/terms/index.post.ts(4 hunks)packages/matrix-identity-server/src/validate/email/requestToken.ts(4 hunks)packages/matrix-identity-server/src/validate/email/submitToken.ts(1 hunks)packages/matrix-identity-server/templates/3pidInvitation.tpl(2 hunks)packages/matrix-invite/Dockerfile(1 hunks)packages/matrix-invite/src/components/Confirmation.svelte(1 hunks)packages/tom-server/templates/3pidInvitation.tpl(1 hunks)packages/utils/src/index.test.ts(7 hunks)packages/utils/src/utils.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/devskim.yml
- packages/matrix-identity-server/templates/3pidInvitation.tpl
- packages/matrix-invite/src/components/Confirmation.svelte
- packages/matrix-identity-server/src/db/sql/pg.ts
- packages/tom-server/templates/3pidInvitation.tpl
- .github/workflows/update-docs.yml
- packages/federated-identity-service/src/testData/ldap/Dockerfile
- packages/federated-identity-service/Dockerfile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T16:08:30.680Z
Learnt from: pm-McFly
Repo: linagora/ToM-server PR: 302
File: packages/amqp-connector/package.json:26-33
Timestamp: 2025-12-16T16:08:30.680Z
Learning: To ensure consistency across the monorepo, advertise the Node.js version by adding an engines.node field to package.json in all packages (e.g., "engines": {"node": "18.20.8"}). This makes the runtime expectation explicit for reviewers and CI. Apply this across all packages in the repo, not just the one mentioned, and plan an upgrade in a future PR once the packages are stabilized and rationalized.
Applied to files:
packages/federated-identity-service/package.json
🧬 Code graph analysis (5)
packages/matrix-identity-server/src/validate/email/submitToken.ts (1)
packages/utils/src/errors.ts (1)
errMsg(144-153)
packages/matrix-identity-server/src/terms/index.post.ts (3)
packages/matrix-identity-server/src/index.ts (1)
getUrlsFromPolicies(59-59)packages/matrix-identity-server/src/terms/index.ts (1)
Policies(11-14)packages/matrix-identity-server/src/types.ts (1)
DbGetResult(70-72)
packages/utils/src/index.test.ts (1)
packages/utils/src/utils.ts (4)
send(15-41)validateParameters(167-175)validateParametersAndValues(186-195)getAccessToken(550-567)
packages/matrix-identity-server/src/index.test.ts (3)
server.mjs (2)
idServer(276-283)app(204-204)packages/crypto/src/index.ts (1)
Hash(8-42)packages/matrix-identity-server/src/terms/index.post.ts (1)
fillPoliciesDB(41-92)
packages/matrix-identity-server/src/validate/email/requestToken.ts (2)
packages/utils/src/errors.ts (1)
errMsg(144-153)packages/utils/src/utils.ts (1)
isValidUrl(528-536)
🪛 Biome (2.1.2)
packages/matrix-identity-server/src/index.test.ts
[error] 1795-1829: Disallow duplicate setup and teardown hooks.
Disallow beforeAll duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (27)
packages/federated-identity-service/templates/3pidInvitation.tpl (1)
5-5: LGTM! Trailing whitespace removed from Subject header.The removal of trailing whitespace from the Subject line is a good practice that improves consistency and prevents potential issues with email clients.
packages/matrix-invite/Dockerfile (1)
24-24: LGTM - cosmetic whitespace cleanup.This formatting improvement aligns with the PR-wide whitespace refinements.
.compose/ldap/Dockerfile (1)
6-19: LDAP infrastructure setup looks solid.The additions properly handle system dependencies, keyring configuration, LDAP package installation, and database initialization. The dual
apt-get updatecalls are appropriate given the repository addition in between.Also applies to: 21-25, 28-32, 34-36
packages/matrix-identity-server/src/db/index.test.ts (2)
27-52: LGTM! Well-designed test helper functions.The helper functions provide clean abstractions for test setup and teardown:
setupIdDb()properly handles async initialization with config mergingcleanupIdDb()ensures proper resource cleanupinsertTestTokens()andinsertAccessTokens()provide reusable test data seedingThese utilities significantly improve test readability and maintainability.
67-690: LGTM! Comprehensive and well-executed async/await refactor.The conversion from callback-based to async/await patterns is thorough and correct:
- All async operations are properly awaited
- Consistent use of helper functions for setup and teardown
- Proper resource cleanup in all tests
- Test assertions correctly validate expected behavior
The timeout test (lines 344-356) uses a 6-second delay, which is appropriate for verifying token expiration behavior.
packages/matrix-identity-server/src/index.test.ts (5)
133-135: LGTM!The test correctly verifies that invalid federated_identity_services are filtered to an empty array, aligning with the PR objective of improving validation.
185-190: LGTM!The mock response's
json()method is correctly updated to return a Promise, which aligns with the actualfetchAPI behavior whereresponse.json()returns a Promise.
562-589: LGTM!Good addition of validation tests for the
send_attemptparameter. These tests cover:
- Non-numeric values (line 571:
'NaN')- Excessively large values (line 585:
99999999999)Both correctly assert 400 responses and verify no mail is sent.
1338-1365: LGTM!The test setup correctly:
- Retrieves the pepper from the database
- Computes the hashed address using the Hash utility
- Inserts test data into the hashes table
- Cleans up after the test (line 1365)
This aligns with the PR's shift to DB-backed hash lookups.
1855-1872: LGTM!Good restructuring of the test suite. The nested
describe('After accepting the terms', ...)block properly:
- Sets up policy acceptance in its own
beforeAllwith properawait- Verifies authentication succeeds after terms acceptance
- Provides clear test isolation from the "before acceptance" tests
packages/matrix-identity-server/src/terms/index.post.ts (4)
16-33: LGTM!The
getUrlsFromPoliciesfunction is now properly exported, enabling reuse across modules as intended by the PR objectives.
35-92: Well-structured upsert implementation with proper error propagation.The
fillPoliciesDBfunction correctly:
- Computes policies from server config
- Uses an upsert pattern (check → update or insert)
- Executes operations concurrently with
Promise.all- Re-throws errors to allow the caller to handle failures
- Includes helpful debug logging
The caution note about overwriting previous acceptance status is a good practice.
105-106: LGTM!Extracting
userIdfromdata.subis correct and simplifies the downstream policy update logic.
126-144: Race condition fix confirmed.The
Promise.allpattern now properly awaits all database updates before sending the response, addressing the previously identified race condition. The error handling correctly sends a single 500 response on failure..github/workflows/njsscan.yml (1)
13-16: LGTM! Well-coordinated workflow trigger updates.The changes extend branch coverage to include
devand addworkflow_callsupport, which aligns with the new job orchestration in pr.yml. The multi-line branch format is more maintainable than the previous single-line array..github/workflows/codeql.yml (2)
16-19: LGTM! Consistent multi-branch and reusable workflow support.The addition of the
devbranch andworkflow_calltrigger aligns with the orchestration changes in pr.yml, enabling CodeQL analysis to run both on direct pushes and when invoked from the PR workflow.
72-72: Good cleanup: formatting fix for paths-ignore.Removing the trailing space improves YAML formatting consistency.
.github/workflows/pr.yml (1)
13-24: Good integration of security and quality checks into the PR workflow.The addition of parallel analysis jobs (CodeQL, DevSkim, njsscan) enhances the CI pipeline. All three referenced workflows are properly updated to support
workflow_call, ensuring correct integration with the PR workflow.packages/matrix-identity-server/src/validate/email/requestToken.ts (3)
39-75: LGTM! Well-structured validation function.The centralized validation logic for
client_secret,next_link, andsend_attemptimproves code organization and reusability. The lower bound validation forsend_attempt(requiring >= 1) correctly addresses the Matrix specification requirement.
127-179: LGTM! Good refactoring with improved error handling.The rename to
fillTableAndSendbetter describes the function's purpose. The error handling now correctly avoids exposing internal error details to clients by usingerrMsg('unknown')without the error message.
196-264: LGTM! Clean refactoring with early validation.The refactored flow validates input upfront using
validateRequestTokenArgs, then proceeds with database operations. The control flow is clearer and error handling is consistent throughout.packages/utils/src/index.test.ts (3)
17-32: LGTM! Good test helper abstraction.The helper functions
expectErrorResponseandexpectNoResponsereduce repetition across test cases and improve readability.
71-86: LGTM! Good coverage for logging behavior.These tests properly verify that the
sendfunction usesdebugfor successful responses (2xx) anderrorfor non-2xx responses.
219-273: LGTM! Comprehensive tests for value validation.The tests cover success path, missing parameters, and invalid values for the new
validateParametersAndValuesfunction.packages/utils/src/utils.ts (3)
15-41: LGTM! Good enhancement for debugging.The optional logger parameter with appropriate log levels (debug for success, error for failures) is a clean addition that aids troubleshooting without requiring code changes.
167-195: LGTM! Clean separation of validation concerns.The
validateParameterswrapper maintains backward compatibility while the newvalidateParametersAndValuesfunction provides extended validation capabilities.
549-567: LGTM! Token extraction logic is correct.The function properly extracts Bearer tokens from the Authorization header and falls back to query parameters. The header takes precedence when present, which is standard OAuth2 behavior.
| "@twake/logger": "*", | ||
| "@twake/logger": "*", |
There was a problem hiding this comment.
Remove the duplicate @twake/logger dependency.
The @twake/logger dependency is declared twice in the dependencies block. In JSON, only the last definition takes effect, making the first entry redundant and potentially confusing.
Apply this diff to remove the duplicate entry:
"@twake/crypto": "*",
"@twake/logger": "*",
- "@twake/logger": "*",
"@twake/matrix-identity-server": "*",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@twake/logger": "*", | |
| "@twake/logger": "*", | |
| "@twake/logger": "*", |
🧰 Tools
🪛 Biome (2.1.2)
[error] 45-45: The key @twake/logger was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
In packages/federated-identity-service/package.json around lines 45-46, there is
a duplicated dependency entry for "@twake/logger"; remove the redundant
duplicate so the dependency appears only once in the dependencies object (leave
the single correct entry), then save the package.json and run your package
manager (npm/yarn/pnpm) to update lockfiles if needed.
| } catch (err) { | ||
| logger.error('JSON error', err) | ||
| logger.error(`Content was: ${content}`) | ||
| send(res, 400, errMsg('unknown', err as string)) | ||
| send(res, 400, errMsg('badJson', err as string)) | ||
| accept = false | ||
| } |
There was a problem hiding this comment.
Fix incorrect error type casting.
Casting err directly as string doesn't convert the Error object to a string—it passes the object reference with an incorrect type assertion. This could result in [object Object] or unexpected behavior in the error message.
Apply this diff to properly convert the error:
} catch (err) {
logger.error('JSON error', err)
logger.error(`Content was: ${content}`)
- send(res, 400, errMsg('badJson', err as string))
+ send(res, 400, errMsg('badJson', err instanceof Error ? err.message : String(err)))
accept = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| logger.error('JSON error', err) | |
| logger.error(`Content was: ${content}`) | |
| send(res, 400, errMsg('unknown', err as string)) | |
| send(res, 400, errMsg('badJson', err as string)) | |
| accept = false | |
| } | |
| } catch (err) { | |
| logger.error('JSON error', err) | |
| logger.error(`Content was: ${content}`) | |
| send(res, 400, errMsg('badJson', err instanceof Error ? err.message : String(err))) | |
| accept = false | |
| } |
🤖 Prompt for AI Agents
In packages/utils/src/utils.ts around lines 68 to 73, the catch block casts the
caught error to a string incorrectly; change it to derive a proper string by
handling unknown error types (e.g. const errStr = err instanceof Error ?
err.message : String(err)), pass that errStr into errMsg('badJson', errStr) for
send, and keep logging the original err object with logger.error for full
context.
| } else { | ||
| if ( | ||
| valuechecks != null && | ||
| content[key] != null && | ||
| !valuechecks[key](content[key]) | ||
| ) { | ||
| wrongValues.push(key) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add null check before invoking value check function.
If valuechecks doesn't contain a validator for a given key, valuechecks[key] will be undefined, and calling it as a function will throw TypeError: undefined is not a function.
Apply this diff to add the missing null check:
} else {
if (
valuechecks != null &&
+ valuechecks[key] != null &&
content[key] != null &&
!valuechecks[key](content[key])
) {
wrongValues.push(key)
}
}🤖 Prompt for AI Agents
In packages/utils/src/utils.ts around lines 122 to 130, the code calls
valuechecks[key](...) without verifying that valuechecks contains a validator
for that key; add a guard so you only call the validator if valuechecks != null
and typeof valuechecks[key] === 'function' (or use optional chaining with a
function check) before invoking it, otherwise treat the value as invalid (push
key to wrongValues) or skip validation as intended.
1 participant
Overview
This PR represents a comprehensive refactoring of the identity service infrastructure, focusing on improved validation, better code reusability, optimized performance, and enhanced maintainability.
Key Changes
Core Refactoring
Validation Utilities (
packages/utils): Added comprehensive validation utilities with regex patterns for Matrix IDs, room IDs, emails, and phone numbersregex.tswith validation functions and extensive test coverageeventTypes.tswith Matrix event type constantssize_limits.mddocumenting Matrix protocol constraintsbadJsonerror typeMatrix Identity Server (
packages/matrix-identity-server):lookup3pidfunction for reusability, eliminating redundant HTTP calls in invitation flowTom Server (
packages/tom-server):Breaking Changes
fillPoliciesDBnow returns a Promise (async operation)Benefits
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.