feat: Upgrade to Notion public integration (OAuth) #630
Conversation
This reverts commit 38721db.
WalkthroughThis pull request introduces significant updates to Notero's authentication and connection management system, transitioning from token-based authentication to an OAuth-based approach for Notion integration. Key changes include the implementation of a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Preferences
participant NotionAuthManager
participant NotionAPI
User->>Preferences: Click "Connect to Notion"
Preferences->>NotionAuthManager: openLogin()
NotionAuthManager->>NotionAPI: Initiate OAuth Flow
NotionAPI-->>NotionAuthManager: Return Encrypted Token
NotionAuthManager->>NotionAuthManager: Decrypt Token
NotionAuthManager->>Preferences: Update Connection Status
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
🤖 Build Bot✅ Successful build ⬇️ Download: notero-0.6.9-674 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (30)
src/content/services/protocol-handler-extension.ts (3)
14-17: Initialize dependencies earlier for reliability.
It could be beneficial to verify that all required dependencies (i.e.,notionAuthManager) are successfully initialized before proceeding to register the extension to ensure that calls on these dependencies don't happen before they're fully set up.
23-30: Handle potential errors more explicitly.
You already log an error if the protocol handler fails to load. Consider providing meaningful user-facing feedback or fallbacks, in case the application continues running in a degraded mode.
70-78: Distinguish error handling for missing vs. invalid parameters.
Here, you throw an error for missing encryption parameters. If the parameters exist but are corrupted or otherwise invalid, consider returning a different error message or code to assist in diagnosing potential problems.src/content/auth/storage.ts (1)
56-67: Warn users about partial data loss.
When the schema parse fails, you passnullinto the pipeline and thus discard that login record. Consider whether you want to surface a more explicit user-facing message or store partial data for debugging.src/content/auth/notion-auth-manager.ts (4)
54-63: Consider concurrency protection for existing sessions.
When you detect an existing OAuth session inopenLogin(), you override it. If there's any chance of parallel calls before the user interacts with the new session, ensure no data is lost or incorrectly overwritten.
65-84: Provide user-friendly logging for missing session.
Whenthis.currentSessionis not set, you throw a generic error. A clearer error message (e.g., “No active session. Please initiate OAuth flow again.”) could help users or logs.
103-111: Ensure fallback for legacy tokens.
If the user has a partially migrated state, confirm that the fallback togetLegacyAuthToken()is always the correct approach. This can help avoid edge cases if the user has partially upgraded to the new OAuth flow.
126-149: Check for cryptographic errors.
WithindecryptTokenResponse, if the wrapped symmetric key is invalid or the ciphertext is malformed, it will throw. You do handle that by throwing an error, but consider more granular error messages (e.g., “Decryption failed: invalid token”) for improved debugging.src/content/prefs/preferences.tsx (5)
50-55: Keep property references consistent.
Ensuring the references to eventManager, notionAuthManager, and UI elements are set in one place (preferably via a constructor or a single initialization block) can maintain clarity on the usage sequence and potential null checks.
161-163: Leverage direct callback references.
You’re using an arrow function forhandleNotionConnectionAdd. Be mindful of scoping or potential rebind concerns if other parts of the code rely on different context. This is likely fine here, but worth verifying event manager usage patterns.
179-180: Minimize spinner usage when possible.
Users may not always require a loading indicator if the request is quick. Consider only showing this spinner after a short threshold if the network call isn’t instantaneous.
199-211: Enhance error handling strategy.
Consider categorizing errors (e.g., network error, invalid workspace, etc.) for more specific user guidance. Right now, you display a generic localized message plus a log. This is good, but more detailed user feedback might improve UX.
292-308: Unify connection logic for upgrade & connect.
upgradeNotionConnectionandconnectNotionshare similar flows. Explore whether you can unify them and pass in a parameter to indicate an “upgrade” scenario, reducing duplication.src/content/utils/index.ts (1)
12-12: Global references can introduce tight coupling
getGlobalNoterocreates a direct dependency on the globalZoteroobject. Keep an eye on testability and potential coupling pitfalls.src/content/utils/base64.ts (1)
3-11: Check for decoding errors
urlSafeBase64Decodegracefully pads missing “=” characters, but if the string is still malformed,atobcan throw an error. Consider adding a try/catch block or checking for an invalid base64 sequence to provide a clearer error message.src/content/utils/elements.ts (1)
25-27: Ensure event target is XUL element.The
isXULElementcheck is handy to confirm we're dealing with a XUL element. Trivia: You might consider an additional null check if there's a risk thattargetis null.src/content/auth/crypto.ts (1)
35-46: RSA key generation length.The use of a 4096-bit RSA key is secure but can be computationally expensive. Consider performance trade-offs, especially if key generation is frequent.
src/content/prefs/preferences.xhtml (2)
19-50: Workspace configuration UI.This newly introduced container clarifies workspace details and offers upgrade/disconnect functionality. Consider adding a tooltip or user guidance text if the user might be unsure about the “upgrade” action or other advanced config steps.
54-54: Consistent styling withnotero-groupbox.Applying the
notero-groupboxclass fosters a consistent look and feel. Ensure no conflicts arise if further styling overrides are applied in custom CSS for specific sections.Also applies to: 76-76
src/content/sync/sync-job.ts (1)
46-51: Preparing sync job with dynamic token retrieval.Retrieving the token upfront is logical for preventing partial or broken sync workflows. Consider retrying or refreshing logic if tokens are short-lived or can expire during the sync process.
src/content/notero.ts (1)
116-122: Asynchronous Notion client creation with required auth token.Good approach to ensure the client is always valid. Consider implementing specialized error handling for token retrieval failures, in case there's a UI or fallback needed.
src/locale/en-US/notero.ftl (1)
64-64: Add consistent punctuation.For consistency, consider adding a period at the end of this sentence:
"Not authorized with Notion. Please connect to Notion in Notero preferences."-notero-error-missing-notion-token = Not authorized with Notion. Please connect to Notion in Notero preferences. +notero-error-missing-notion-token = Not authorized with Notion. Please connect to Notion in Notero preferences.(Note: The line remains the same, but adding a period at the end is recommended for uniformity in error messages.)
types/xpcom.d.ts (3)
35-35: Document any exceptions.The new
getProtocolHandlermethod may throw an exception ifschemeis unrecognized. Consider documenting or handling any potential errors that might emerge from invalid or unsupported schemes.
99-105: Prompt correctness and localized strings.
confirmmay need localized titles and messages. Guarantee that consumers of this interface properly handle localized strings, especially if custom UI is rendered.
222-234: Constructor type clarity.This new
Constructortype for dynamic instantiation is flexible. Clearly document usage in the codebase where these dynamic creations occur, to ensure debugging remains manageable.src/content/services/sync-manager.ts (2)
20-21: Consider explicit initialization.
private getNotionAuthToken!: () => Promise<string>;is an example of definite assignment, which TypeScript allows. For clarity, you may consider a constructor-based assignment or explicit initialization to help future maintainers trace dependencies more easily.
262-262: Integration with performSyncJob.By passing
this.getNotionAuthTokentoperformSyncJob, you are centralizing OAuth retrieval. That is a good design pattern. Ensure the function properly handles authentication failures (e.g., expired tokens), prompting the user to reconnect or refresh.types/zotero.d.ts (2)
462-464: Check private field accessibility.
_extensions: Record<\zotero://${string}`, ZoteroProtocolHandlerExtension>;is a private field pattern. Ensure that only internal modules modify_extensions` and that you provide a stable public API for extension registration if needed.
466-470: Validate unknown returns in newChannel.
newChannelreturns an unknown type, which can hamper type-checking. If it's always a specific channel interface, consider specifying it for better compile-time validation.src/content/services/__tests__/sync-manager.spec.ts (1)
140-140: Consider mockingNotionAuthManager.In unit tests, to prevent external side effects and keep tests focused on the
SyncManager, consider mockingNotionAuthManagersimilarly to other dependencies.
📜 Review details
Configuration used: CodeRabbit 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 (31)
.zed/tasks.json(1 hunks)package.json(1 hunks)src/content/auth/crypto.ts(1 hunks)src/content/auth/index.ts(1 hunks)src/content/auth/notion-auth-manager.ts(1 hunks)src/content/auth/storage.ts(1 hunks)src/content/notero.ts(3 hunks)src/content/prefs/preferences.tsx(4 hunks)src/content/prefs/preferences.xhtml(2 hunks)src/content/services/__tests__/sync-manager.spec.ts(4 hunks)src/content/services/event-manager.ts(2 hunks)src/content/services/index.ts(1 hunks)src/content/services/preference-pane-manager.ts(1 hunks)src/content/services/protocol-handler-extension.ts(1 hunks)src/content/services/service.ts(1 hunks)src/content/services/sync-manager.ts(2 hunks)src/content/style/preferences.css(1 hunks)src/content/sync/notion-client.ts(1 hunks)src/content/sync/sync-job.ts(2 hunks)src/content/utils/base64.ts(1 hunks)src/content/utils/elements.ts(2 hunks)src/content/utils/get-global-notero.ts(1 hunks)src/content/utils/get-main-window.ts(1 hunks)src/content/utils/index.ts(1 hunks)src/locale/en-US/notero.ftl(2 hunks)src/prefs.js(0 hunks)types/jsm.d.ts(1 hunks)types/utils.d.ts(1 hunks)types/xpcom.d.ts(7 hunks)types/xul.d.ts(2 hunks)types/zotero.d.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/prefs.js
✅ Files skipped from review due to trivial changes (1)
- src/content/auth/index.ts
🔇 Additional comments (68)
src/content/services/protocol-handler-extension.ts (1)
44-67: Consider concurrency or re-entrancy for doAction.
SincedoActionandnewChannelare async-like flows, ensure no conflicts arise (e.g., parallel calls). If there's a potential for multiple invocations on the same resource, you might want to gate certain operations or handle repeated calls gracefully.src/content/auth/storage.ts (2)
70-83: Avoid partial updates without transaction-like handling.
InsaveConnection(), you modify the existing login if found or add a new one otherwise. If either step fails mid-operation, the system might end up in an inconsistent state. Investigate whether a transaction-like approach or rollback logic is feasible.
85-94: Review potential concurrency around removeConnection.
While unlikely, a user might remove a connection while another process expects that connection to exist. Confirm that no concurrency issues could arise with chase-up calls or references to the same login.src/content/auth/notion-auth-manager.ts (1)
86-92: Clarify session-based vs. persistent data retrieval.
getAllConnections()is returning persisted data, while the code also stores ephemeral session info in memory (currentSession). If a session-based approach is eventually used for connection references, confirm that the logic remains consistent.src/content/prefs/preferences.tsx (2)
91-99: Validate repeated event listeners.
You add a'notion-connection.add'event listener on init, and remove it on unload. Confirm that there’s no scenario where multiple inits occur without a matching unload, leading to multiple handlers.
165-167: Double-check for conflicts between new & legacy tokens.
IfgetFirstConnection()returns a connection but a legacy token also exists, confirm which takes precedence in the UI or sync logic.types/utils.d.ts (1)
1-4: Type alias looks good.
This conditional type effectively extracts function properties from a given type. It’s simple, yet powerful, and should be quite useful in utility scenarios.src/content/utils/get-main-window.ts (1)
1-5: Looks great!Exporting a helper function to retrieve the main window ensures that other modules in your codebase can consistently access the main Zotero window or gracefully handle an unavailable environment. The method is straightforward, and the error handling is clear.
src/content/services/index.ts (2)
1-1: No concerns about the updated export statement.Exporting both types
ServiceandServiceParamsfrom theservicemodule helps keep type definitions consolidated and easy to consume in other parts of the codebase.
5-5: Verifying the newly exported component.Exporting
ProtocolHandlerExtensionbroadens the module's capabilities. Please ensure you have the proper documentation or usage examples for other team members.✅ Verification successful
Export of
ProtocolHandlerExtensionis properly used and integratedThe verification shows that
ProtocolHandlerExtensionis:
- Properly defined as a service class in
protocol-handler-extension.ts- Used as part of the core services in
notero.ts- Implements the
ZoteroProtocolHandlerExtensiontype defined inzotero.d.ts- Integrated into the application's service initialization flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm usage of ProtocolHandlerExtension throughout the codebase rg -A 5 "ProtocolHandlerExtension"Length of output: 2385
src/content/sync/notion-client.ts (1)
Line range hint
10-16: Transition to parameterized authToken is well-structured.Passing in
authTokenas a parameter clearly decouples token retrieval from the client creation logic, improving overall flexibility and testability. Make sure that all call sites have the correct token to avoid unexpected authentication failures.src/content/utils/get-global-notero.ts (1)
1-12: A clean approach to accessing the global Notero instance.This utility neatly encapsulates global object access, improving code readability elsewhere. The error message is informative and ensures callers handle missing global objects.
src/content/services/preference-pane-manager.ts (1)
17-17: Consider handling undefinedpaneIDReturning the result is a helpful improvement, but if
paneIDis unexpectedlyundefined,Zotero.Utilities.Internal.openPreferencesmight fail. It may be worth checking forpaneIDvalidity and, if invalid, logging a warning or throwing an appropriate error.src/content/services/service.ts (3)
1-1: Imports look correctBringing in
NotionAuthManagerfrom../authto manage authentication is well-aligned with the PR's objectives.
8-8: Ensure robust error handling for NotionAuthManagerNow that
notionAuthManageris part ofDependencies, confirm that downstream consumers properly handle potential errors thrown by any Notion authentication workflow.
12-12: Expanded generic parameter ensures flexibilityChanging the default generic parameter from
nevertokeyof Dependenciesis a clean approach if you need more flexible typed access to dependencies. Verify that no existing code that relied on the previous default is impacted.src/content/utils/index.ts (3)
1-1: Exporting Base64 utilities is consistent with usageExposing
urlSafeBase64DecodeandurlSafeBase64Encodehere is appropriate, allowing reuse across the codebase.
8-9: Check DOM environment assumptions
isXULElementandisXULElementOfTyperely on a XUL-based environment. Ensure that these functions fail gracefully or remain properly scoped should the environment evolve (e.g., newer versions of Zotero or Firefox).
15-15: Verify multi-window scenarios
getMainWindowworks well if only one main Zotero window is open. If multiple windows are in play, confirm that retrieving the first main window is intended behavior.src/content/utils/base64.ts (1)
13-24: String vs. ArrayBuffer conversion logic
urlSafeBase64Encodehandles both string andArrayBufferinputs nicely. Ensure consistent usage and confirm that all call sites explicitly handle or expect the URL-safe version without trailing padding.types/jsm.d.ts (1)
20-22: Good addition to theServicesinterface.The
loginsandpromptproperties will allow seamless integration with the login and prompt services in this refactor. Ensure you handle any potential security prompts and user consent explicitly when working withprompt.src/content/utils/elements.ts (3)
4-8: Consistent naming of type parameters.Renaming
NametoNbrings consistency and clarity. This helps avoid confusion with similarly named local variables or types.
11-15: Maintain parallel structure betweencreateHTMLElementandcreateXULElement.Both functions now share a similar structure and naming convention for the type parameter, which is a good practice for readability and maintainability.
29-34: Extend usage checks with tag validation.
isXULElementOfTypeelegantly builds onisXULElementto further verify that an element is of a particular tag. This improves type safety and reduces casting.types/xul.d.ts (3)
16-18: Addition ofCommandEventis appropriate.Allowing only these properties from
Eventworks well for simpler command triggers. Confirm that you don't need other properties like event phase or composed path in your use case.
23-25: IntroducingLabelElementfor XUL.Adding a dedicated interface for the label element clarifies how we access and manipulate label text. This is helpful for strongly typed UI interactions.
77-77:XULElementTagNameMapupdated with new label element.This ensures that when creating or querying
<label>tags, theirLabelElementinterface is recognized. It enhances tooling (e.g., IntelliSense) and reduces accidental misuse.src/content/auth/crypto.ts (7)
1-2: Confirm usage of local import forgetMainWindow().This file depends on
getMainWindow()from../utils. Make sure there are no circular dependencies or issues if other modules rely oncrypto.ts.
3-9: Comprehensive documentation block.This doc comment is well-written and clarifies parameters and return types for
decrypt. Maintaining robust doc comments helps new contributors.
10-20: AES-GCM decryption approach.The usage of
AES-GCMis appropriate for confidentiality and integrity. Ensureivlength is set to recommended best practices (12 bytes is typical).
22-26: Exporting key for external usage.
exportKey('spki', key)is good for a public key. Verify that the type of key being passed in can actually be exported in SPKI format (public keys only).
31-34: Helpful doc comment.The description gives a clear overview of the key pair generation process.
48-53: Ensure thatwrappedKeyis never logged.It might be tempting to debug by logging
wrappedKey, but this can compromise security. Confirm that your code does not log sensitive data.
54-67: Unwrapping AES-GCM key with RSA-OAEP.Good to see a clearly defined key usage for
['decrypt']. Keep in mind that if encryption is needed by the unwrapped key, usage must include['encrypt', 'decrypt'].src/content/services/event-manager.ts (2)
3-3: Ensure type import is correct and used consistently.The
NotionConnectiontype import appears appropriate. Confirm that'../auth'has the correct export ofNotionConnectionand ensure it is used consistently wherever you handle these connections.
37-38: New event handlers for Notion connections seem correct.These new events look well-defined. Make sure you dispatch them consistently in places where Notion connections are added or removed, to maintain a uniform event-driven architecture.
src/content/prefs/preferences.xhtml (4)
6-6: Initialization on<groupbox>load.Calling
notero.preferences.init()in theonloadof<groupbox>provides a more immediate initialization. Verify that no race conditions occur if any earlier UI setup depends onpreferences.init()being completed.
10-15: Conditional connect button display.Hiding the connect button by default and revealing it only when needed is a good UX approach. Be sure to keep logic for toggling the
hiddenproperty in sync with the actual authentication state.
18-18: Spinner for loading or connecting to Notion.The spinner provides a helpful visual cue. Confirm that it doesn't remain visible in scenarios where notifications or callbacks fail, causing indefinite spinning.
51-51: Error label for Notion connections.Displaying errors inline helps with immediate feedback. Check that the label is visible at the appropriate times and properly cleared or hidden once errors are resolved.
src/content/sync/sync-job.ts (2)
29-29: Parameter addition to accept Notion authentication token retrieval.Adding
getNotionAuthTokendirectly intoperformSyncJobis a clean approach, improving the testability of token retrieval. Verify that all call sites pass a function returning a valid token and handle re-authentication if needed.
39-39: Refactor to fetchSyncJobParamswith the new parameter.This ensures centralized retrieval of Notion authentication tokens. Validate that any existing error handling (e.g., token expiry) is gracefully handled here.
src/content/notero.ts (5)
3-3: Dependency import forNotionAuthManager.Double-check that
NotionAuthManagerencapsulates all necessary logic (refresh, token storage, etc.) so that external classes can remain unaware of token intricacies.
18-20: Expanding public properties for event and auth management.Exposing
eventManagerandnotionAuthManagercan facilitate a more modular approach. However, consider potential side effects if external modules mutate them or rely on them extensively.
27-27: Service composition including the newNotionAuthManager.Including the
NotionAuthManagerin the service array ensures a cohesive lifecycle. Confirm no circular dependencies arise between these services.Also applies to: 32-34
53-55: Injecting the new auth manager intoServicedependencies.This design centralizes all Notion auth interactions through the
NotionAuthManager. Make sure services that need re-auth flows can also access it safely.
125-128: Performing duplicate-finding after client retrieval.This ensures the logic always leverages a valid token-based client instance. If
getNotionClient()fails, consider returning an empty result or throwing a more specialized error for better user feedback.src/locale/en-US/notero.ftl (1)
12-20: Use consistent placeholders and confirm variable usage.These new strings effectively convey the new approach to connecting with Notion. Ensure the
$workspace-namevariable is always set or gracefully handled, especially if multiple workspaces are addressed in the new public integration. Uninitialized variables could lead to user confusion.types/xpcom.d.ts (6)
47-64: Solid interface definition.The
nsILoginInfointerface covers the necessary fields for login management. No immediate coding concerns. If there's a possibility of optional fields (besidesformActionOriginandhttpRealm), consider marking them as optional to ensure robust usage across the codebase.
66-79: Evaluate concurrency of asynchronous operations.
addLoginAsyncandsearchLoginsAsyncopen the door to concurrency issues if multiple writes occur simultaneously. Confirm that the underlying implementation innsILoginManagerhandles these gracefully, or add comments clarifying thread safety.
81-87: Check initialization details.
nsILoginMetaInfois crucial for tracking timestamps and usage counts. Ensure that new entries settimeCreatedor other fields appropriately. The correct initialization logic should prevent undefined or zeroed-out timestamps that could hinder data auditing.
107-107: Typographical correctness confirmed.Mapping
nsIProtocolHandlertonsISupportsis correct if the actual protocol handler extendsnsISupports. No further changes required.
124-124: Optional field usage.
wrappedJSObject?: object;is a common pattern. No issues identified here unless you need stricter typing on the wrapped JS object for advanced reflection or manipulation.
11-12: Ensure correct usage references.You've declared new interfaces in
InterfacesfornsILoginInfoandnsILoginManager. Verify that any references to these within the codebase are correctly importing and typing them, avoiding shadowed or nondescript references.✅ Verification successful
All references to nsILoginInfo and nsILoginManager are correctly typed and imported
The verification shows that all references to
nsILoginInfoandnsILoginManagerare properly used:
- In
types/jsm.d.ts,nsILoginManageris correctly referenced through the XPCOM namespace- In
src/content/auth/storage.ts, both interfaces are properly imported and used with the XPCOM namespace- The interfaces are well-defined in
types/xpcom.d.tswith appropriate type information and method signatures- No shadowed or nondescript references were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search references to nsILoginInfo and nsILoginManager rg -A 5 "nsILoginInfo|nsILoginManager"Length of output: 2898
src/content/services/sync-manager.ts (1)
26-32: Dependency injection verified.Receiving
notionAuthManageris an excellent approach for testability. Confirm that consistent error checks or fallback logic are performed if the manager is unavailable or fails to provide a valid token.src/content/services/__tests__/sync-manager.spec.ts (4)
10-10: Looks good.Importing
NotionAuthManagerfrom../../authis consistent with the new OAuth-based approach. No immediate concerns here.
143-143: Dependency injection follows best practices.This pattern cleanly injects
eventManagerandnotionAuthManager. The approach is consistent and maintainable.
187-187: Confirm argument ordering formockedPerformSyncJob.Using index
2to reference the main window parameter indicates the signature has changed. Ensure the new parameter order is correctly reflected in all relevant tests and mocks.
196-196: Keep tests consistent with the updated parameter order.Same as above. Confirm that the last call index
[2]is correct for referencing the main window.src/content/style/preferences.css (3)
1-3: Validate CSS variable usage.Using
var(--accent-red)for.notero-errorensures consistent theming. Confirm that--accent-redis defined for both light/dark themes.white-space: normal;is suitable for multi-line messages, but considerpre-wrapif you expect line breaks in error text.
6-7: Improved spacing.Applying
margin-inline-end: 5px;to all but the last child refines layout consistency in group boxes.
10-11: Good use of max-width.Restricting child elements to
max-width: 100%prevents overflow. This simplifies responsiveness and ensures clean layouts..zed/tasks.json (4)
44-44: Non-intrusive reveal setting for "Test all (once)".Switching
"reveal"to"no_focus"ensures the task won't steal focus—an improved developer experience when quickly running tests.
49-49: Non-intrusive reveal setting for "Test all (watch)".Same rationale: avoiding forced terminal focus delivers a smoother workflow.
55-55: Non-intrusive reveal setting for "Test $ZED_STEM".Again, this is a consistent approach to keep the terminal in the background while tests run.
61-61: Non-intrusive reveal setting for "Test $ZED_SYMBOL".Maintaining this behavior across all test tasks keeps the workspace more organized.
package.json (2)
58-58: Retainingeventemitter3at "^5.0.1" looks stable.No issues with this version from a quick glance, though always monitor for security patches.
59-59: New dependency onzod.Great choice for runtime schema validation. Ensure thorough test coverage to confirm correct usage and reduce redundancy with any existing type validations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/content/auth/crypto.ts (1)
50-69: Add type validation for unwrapping key.While the implementation is secure, consider adding a type check to ensure the unwrapping key is private:
export function unwrapKey( wrappedKey: BufferSource, unwrappingKey: CryptoKey, ): Promise<CryptoKey> { + if (unwrappingKey.type !== 'private') { + throw new Error('Unwrapping key must be a private key'); + } + return getMainWindow().crypto.subtle.unwrapKey( 'raw', wrappedKey,src/content/auth/notion-auth-manager.ts (2)
40-40: Consider making OAuth URL configurable.The OAuth login URL is hardcoded. Consider making it configurable through environment variables or settings to support different environments (development, staging, production).
128-151: Enhance error handling in token decryption.While the implementation is secure, consider adding more specific error handling:
private async decryptTokenResponse( unwrappingKey: CryptoKey, { key, iv, tokenResponse }: EncryptedTokenResponse, ): Promise<OauthTokenResponse> { - const symmetricKey = await unwrapKey( - urlSafeBase64Decode(key), - unwrappingKey, - ); - const jsonResponseBuffer = await decrypt( - symmetricKey, - urlSafeBase64Decode(iv), - urlSafeBase64Decode(tokenResponse), - ); + try { + const symmetricKey = await unwrapKey( + urlSafeBase64Decode(key), + unwrappingKey, + ); + const jsonResponseBuffer = await decrypt( + symmetricKey, + urlSafeBase64Decode(iv), + urlSafeBase64Decode(tokenResponse), + ); - const decoder = new TextDecoder(); - const jsonResponse = decoder.decode(jsonResponseBuffer); + const decoder = new TextDecoder(); + const jsonResponse = decoder.decode(jsonResponseBuffer); - const parsedResponse = JSON.parse(jsonResponse); + const parsedResponse = JSON.parse(jsonResponse); + if (!isObject(parsedResponse)) { + throw new Error('Response is not an object'); + } + if (typeof parsedResponse.access_token !== 'string') { + throw new Error('Invalid access_token format'); + } + return parsedResponse as OauthTokenResponse; + } catch (error) { + throw new Error(`Failed to decrypt token response: ${error.message}`); + } - if (isObject(parsedResponse) && parsedResponse.access_token) { - return parsedResponse as OauthTokenResponse; - } - - throw new Error('Invalid access token response'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/content/auth/crypto.ts(1 hunks)src/content/auth/notion-auth-manager.ts(1 hunks)
🔇 Additional comments (5)
src/content/auth/crypto.ts (3)
22-31: LGTM! Safe public key export implementation.The implementation correctly:
- Uses SPKI format which is appropriate for public keys
- Includes a runtime type check to prevent accidental private key export
33-48: LGTM! Strong key generation parameters.The implementation uses secure parameters:
- 4096-bit RSA key length (strong)
- SHA-256 for hashing
- Standard public exponent (65537)
- Appropriately restricted key usage
3-20: LGTM! Secure implementation of decryption.The implementation correctly uses AES-GCM with proper IV handling. The Web Crypto API usage ensures secure cryptographic operations.
Let's verify no test files accidentally expose sensitive data:
✅ Verification successful
The previous search might have failed due to non-existent test files or pattern matching issues. Let's first verify if there are any test files and then broaden our search to include any potential crypto-related test files or hardcoded test data.
Let's examine the complete crypto implementation and verify if there are any dedicated test files for it, as well as any test files that might be using the crypto functionality.
Crypto implementation is secure with no exposed test vectors
After thorough verification of the codebase:
- The crypto.ts file contains only the implementation code with no test utilities or hardcoded values
- No dedicated crypto test files were found
- No imports of the crypto module were found in any test files
- The only key-related references in tests are for Zotero citation keys, which are unrelated to cryptographic operations
The implementation remains secure with proper use of Web Crypto API, and there are no test files that could potentially expose sensitive cryptographic material.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential test vectors or hardcoded encrypted data rg -g '**/*.{test,spec}.{ts,js}' -A 5 'AES-GCM|encrypt|decrypt'Length of output: 65
Script:
#!/bin/bash # Let's try a broader search for test files first fd ".*\.(test|spec)\.(ts|js)$" # Then search for any crypto-related test files fd "crypto.*\.(ts|js)$" # Also search for any hardcoded test vectors or keys in the entire codebase rg -i "test.*key|test.*vector|mock.*key|mock.*crypto"Length of output: 1181
Script:
#!/bin/bash # Let's check the content of crypto.ts for any test utilities or mock data cat src/content/auth/crypto.ts # Let's also check if there are any test files specifically for crypto fd -g "*crypto*.test.*" -g "*crypto*.spec.*" # Check for any imports of the crypto module in test files rg -l "import.*crypto" $(fd ".*\.(test|spec)\.(ts|js)$")Length of output: 2877
src/content/auth/notion-auth-manager.ts (2)
67-86: LGTM! Secure token handling implementation.The implementation:
- Validates session state
- Properly handles token decryption
- Cleans up legacy tokens
- Notifies UI of changes
88-126: LGTM! Well-structured token management.The implementation provides:
- Clear distinction between optional and required tokens
- Proper backward compatibility with legacy tokens
- Appropriate error handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/content/auth/crypto.ts (1)
54-56: Potential collision check for nonces.
While the probability of collisions in a 16-byte random nonce is extremely low, ensure that repeated or reused nonces do not inadvertently break any part of your OAuth handshake flow.I can help by generating a script to ensure your code never reuses nonce values in a single session. Would you like me to create a PR for that?
src/content/auth/notion-auth-manager.ts (5)
61-75: Use a random state parameter for better CSRF mitigation.
You’re currently combining the public key with a base64-encoded nonce as the OAuth state parameter. This is good, but randomizing the entire state (rather than only half) can further reduce risk.
77-98: Securely handle the window of time between session creation and token retrieval.
If the user restarts the app or a new session unexpectedly overwrites the existing one, consider storing minimal session info (e.g.,nonce) to local storage so that a returning token response can still be handled if the app is restarted mid-flow.
108-110: Legacy token fallback strategy.
The fallback togetLegacyAuthToken()is good for backward compatibility. Ensure this logic remains only as long as necessary. Plan a deprecation strategy to eventually remove older tokens.
135-138: Cleanly handle or log missing connections.
If the connection to remove is not in storage, ensure you log or handle that gracefully. Currently, you assume it exists; if not, theremoveConnectioncall might fail silently.
140-150: Enhance error message clarity.
When throwing "Invalid access token parameters," consider providing details about which parameter is specifically missing to ease debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/content/auth/crypto.ts(1 hunks)src/content/auth/notion-auth-manager.ts(1 hunks)src/content/services/protocol-handler-extension.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/content/services/protocol-handler-extension.ts
🔇 Additional comments (5)
src/content/auth/crypto.ts (3)
10-20: Validate data length before decryption.
Although AES-GCM can handle variable-length data, it might be prudent to validate thativandencryptedDatameet expected lengths or patterns, to detect malformed or possibly corrupted inputs earlier.As a follow-up, consider verifying if any external user-generated input flows into this function, potentially requiring stricter validation.
27-31: Ensure no accidental private key exports.
When exporting a key, confirm that the provided key is guaranteed to be a public key. The code already checkskey.type, but if there's any scenario where a private key is mistakenly passed, it could pose a security risk.
37-48: Key usage scopes.
ThegenerateKeyPairfunction sets['wrapKey', 'unwrapKey']as usages. Verify if you also needencrypt/decryptorsign/verifyin other scenarios. If not, this usage scope is appropriate.Consider running a quick repository search for usage of the generated key pair to confirm that no additional key operations are needed:
✅ Verification successful
Key usage scope is appropriate
The current usage scope['wrapKey', 'unwrapKey']is exactly what's needed. The RSA key pair is used solely for key exchange, where the private key unwraps an AES-GCM key, which then handles the actual decryption. No additional key operations like encrypt/decrypt or sign/verify are required for this auth flow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching references to `generateKeyPair()` usage rg -A 5 "generateKeyPair"Length of output: 1202
Script:
#!/bin/bash # Search for crypto operations in auth-related files rg -A 5 "wrap|encrypt|decrypt|sign|verify" src/content/auth/Length of output: 5046
src/content/auth/notion-auth-manager.ts (2)
127-133: Consider concurrency concerns with multiple connections.
When removing all connections, each removal triggers asynchronous calls. If new connections are added in parallel, consider whether concurrency might produce race conditions or partial states.
152-175: Potential data exposure ifisObject(parsedResponse)fails.
If theparsedResponseis not an object, or malicious data is encountered, you throw an error. Ensure logs do not inadvertently leak the raw response in production.
| export function unwrapKey( | ||
| wrappedKey: BufferSource, | ||
| unwrappingKey: CryptoKey, | ||
| ): Promise<CryptoKey> { | ||
| return getMainWindow().crypto.subtle.unwrapKey( | ||
| 'raw', | ||
| wrappedKey, | ||
| unwrappingKey, | ||
| { name: 'RSA-OAEP' }, | ||
| { name: 'AES-GCM' }, | ||
| false, | ||
| ['decrypt'], | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add error handling for cryptographic operations
The decryptTokenResponse method should catch and handle potential failures from unwrapKey and decrypt operations. Add a try-catch block to provide meaningful error messages for cryptographic failures.
private async decryptTokenResponse(...): Promise<OauthTokenResponse> {
try {
const symmetricKey = await unwrapKey(...);
const jsonResponseBuffer = await decrypt(...);
// ... rest of the code
} catch (error) {
throw new Error(`Failed to decrypt token response: ${error.message}`);
}
}🔗 Analysis chain
Validate unwrapKey output for non-null.
After unwrapping the key, confirm that subsequent code gracefully handles potential failures, such as incorrect key usage or a corrupted ciphertext.
Use a quick script to track possible errors from unwrapKey calls:
Ensure error handling is in place if unwrapKey fails.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for unwrapKey usage
rg -A 5 "unwrapKey"
Length of output: 1548
Script:
#!/bin/bash
# Get more context around the unwrapKey usage in notion-auth-manager.ts
rg -B 5 -A 10 "const symmetricKey = await unwrapKey"
# Also check for try-catch blocks in the same file
ast-grep --pattern 'try {
$$$
unwrapKey($$$)
$$$
}'
Length of output: 1202
Script:
#!/bin/bash
# Get the complete class implementation to check error handling
ast-grep --pattern 'class NotionAuthManager {
$$$
}'
# Also search for calls to decryptTokenResponse
rg -B 5 "decryptTokenResponse"
Length of output: 971
|
Included in release v1.0.0 |
Upgrade to use a Notion public integration (OAuth) instead of internal integration.1 This means users will no longer need to create their own internal integrations and instead can connect using a simpler OAuth flow.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
Dependencies
zodlibrary for schema validation.Footnotes
See Notion authorization docs ↩