New Oauth Implementation #267
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c3e64be in 3 minutes and 5 seconds
More details
- Looked at
977lines of code in13files - Skipped
1files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/app-config/_generateConnectorLists.ts:90
- Draft comment:
Consider clarifying the TODO regarding automated package.json generation for new packages. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking for clarification on a TODO, which is not a direct code suggestion or a request for a specific test. It seems to be asking for more information or confirmation, which violates the rules.
2. apps/app-config/_generateConnectorLists.ts:183
- Draft comment:
Ensure cnext connector naming is consistent; review the logic that prepends 'cnext-' to dir names. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. connectors/cnext/server.ts:113
- Draft comment:
Resolve the @ts-expect-error in preConnect; suppressing TS errors can hide genuine type issues. - Reason this comment was not posted:
Marked as duplicate.
4. connectors/cnext/server.ts:134
- Draft comment:
Similarly, resolve the @ts-expect-error in postConnect to ensure type safety. - Reason this comment was not posted:
Marked as duplicate.
5. packages/engine-backend/context.ts:87
- Draft comment:
Logging entire connector objects with JSON.stringify may expose sensitive data; consider redacting sensitive fields. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
This is a validation check that only runs during initialization when a connector is misconfigured. The connector object here is just configuration data, not runtime data. The error is thrown immediately after logging. This is a development-time error, not a production scenario. The suggestion to redact fields seems overly cautious for this context.
I could be wrong about the connector object not containing sensitive data - there might be API keys or credentials in the connector config that shouldn't be logged.
Even if there are sensitive fields, this is a fatal error during initialization that indicates invalid configuration. Logging the full object in this case is valuable for debugging and the risk is low since it's not in normal operation.
The comment should be deleted. The logging is appropriate for this initialization-time validation check, and adding field redaction would reduce the usefulness of the error logging.
6. connectors/cnext/server.ts:56
- Draft comment:
State validation is skipped; ensure state is properly validated to prevent CSRF attacks. - Reason this comment was not posted:
Marked as duplicate.
7. connectors/cnext/server.ts:113
- Draft comment:
Avoid using @ts-expect-error if possible; improve typings so that the code passes type-checking. - Reason this comment was not posted:
Marked as duplicate.
8. packages/engine-backend/context.ts:87
- Draft comment:
Logging full connector details using JSON.stringify may expose sensitive info; review what gets logged on errors. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_OOiED89KBPRVdzIh
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| description: 'Full access to files and folders in your Google Drive', | ||
| }, | ||
| ], | ||
| handlers: {}, // TODO: figure out how to not make this required |
There was a problem hiding this comment.
Consider implementing default handlers or marking them as optional explicitly instead of an empty object.
| handlers: {}, // TODO: figure out how to not make this required | |
| handlers?: {}, // TODO: figure out how to not make this required |
| } | ||
|
|
||
| // TODO: validate state | ||
| console.warn('SKIPPING OAUTH STATE VALIDATION: ', state) |
There was a problem hiding this comment.
Skipping OAuth state validation (line 56) may pose a security risk; consider properly validating the state parameter.
| .describe('Default scopes for the OpenInt platform connector app'), | ||
| auth_scope_separator: z | ||
| .string() | ||
| .default(' ') |
There was a problem hiding this comment.
Default auth_scope_separator (' ') does not match its description (defaults to comma).
| .default(' ') | |
| .default(',') |
connectors/cnext/server.ts
Outdated
| authorization_request_url: connectorDef.authorization_request_url, | ||
| auth_params: connectorDef.auth_params, | ||
| connector_config: connectorConfig, | ||
| redirect_uri: 'http://localhost:4000/connect/callbacknew', // TODO: should this be default or dynamic from context.redirect_uri |
There was a problem hiding this comment.
Hard-coded redirect URI ('http://localhost:4000/connect/callbacknew') is used; consider making it dynamic or configurable for production.
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| @@ -1,4 +1,5 @@ | |||
| // generated by _generateConnectorLists.ts. Do not modify by hand | |||
| import {default as connectorGoogledrive} from '@openint/cnext' | |||
There was a problem hiding this comment.
The import path '@openint/cnext' seems incorrect compared to the pattern used for other connectors. It should likely be '@openint/connector-googledrive/def' to maintain consistency with other connector imports.
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
| @@ -0,0 +1,10 @@ | |||
| import { | |||
There was a problem hiding this comment.
Missing curly brace closure - the import statement has an opening curly brace but lacks proper closure on the same line. In TypeScript/JavaScript, it's generally better style to have the opening brace on the same line as the import keyword to avoid ASI (Automatic Semicolon Insertion) issues.
💬 Reply with
/praiseto tell me that this comment was useful.
Is this comment irrelevant to your project? Reply with
/ignoreto no longer receive comments like this in the future.
|
😱 Found 2 issues. Time to roll up your sleeves! 😱 |
…d if code and state passed
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8c7e1b9 in 1 minute and 16 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. packages/engine-backend/router/customerRouter.ts:385
- Draft comment:
Risk of overriding an existing id in int.config. Ensure int.config does not already include an id key, as spreading it after setting {id: ccfgId} may inadvertently mask intended settings. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment raises a valid concern about object property ordering and potential overrides. However, this seems to be an intentional change to ensure ccfgId is used as the id. The code appears to be deliberately setting id first, then spreading the rest of the config. Without seeing int.config's type definition or the connector interface, we can't be certain if this is actually a problem.
I may be missing important type information that would make this a real issue. The comment could be pointing out a genuine risk if int.config is expected to provide its own id that shouldn't be overridden.
However, the code change appears intentional and deliberate - setting id first then spreading config suggests the author wants ccfgId to take precedence. Without strong evidence that this is incorrect, we should trust the author's intention.
The comment raises a speculative concern without clear evidence of an actual problem. We should remove it since it's asking the author to "ensure" something rather than pointing out a definite issue.
2. packages/engine-backend/router/customerRouter.ts:385
- Draft comment:
Potential override: spreading int.config after inserting ccfgId may overwrite the id. Consider reversing the spread order if ccfgId should take precedence. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/engine-backend/router/customerRouter.ts:24
- Draft comment:
Typographical error: In the comment (line 24) of zConnectTokenPayload, "dont' need it..." should be corrected to "don't need it..." for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_XUQr3TwF2I2YGIr2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cdc34d3 in 29 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. packages/engine-frontend/hocs/WithConnectorConnect.tsx:9
- Draft comment:
Removed unused import 'makeId'. Verify it’s not needed elsewhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/engine-frontend/hocs/WithConnectorConnect.tsx:9
- Draft comment:
Good removal of unused 'makeId'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. packages/engine-frontend/hocs/WithConnectorConnect.tsx:272
- Draft comment:
Typo found: in the comment, 'accessiblity' should be corrected to 'accessibility'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_tN2cpZlxrhT8eyB2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Introduces a new OAuth implementation for Google Drive, updating connector management, configuration, schema, server, and frontend integration.
connectors/cnext/googledrive/index.ts.createNativeOauthConnectinkits/cdk/internal/oauthNative.tsfor handling OAuth popups.OAuthCallbackinapps/web/app/connect/(oauth)/callback/page.tsxto close window after successful OAuth._generateConnectorLists.tsto include Google Drive in connector lists.connectors.def.ts,connectors.merged.ts, andconnectors.server.tsto include Google Drive.meta.jsto add Google Drive connector metadata.@openint/cnextpackage inpackage.jsonfor Google Drive connector.next.config.jsto handle new connector paths.zConnectorConfigandzOAuthConnectorDefinconnectors/cnext/def.ts.generateConnectorDefandgenerateConnectorServerV1inconnectors/cnext/schema.tsandserver.ts.WithConnectorConnect.tsxto usecreateNativeOauthConnectfor native OAuth handling.Cross-Origin-Opener-Policyheader innext.config.jsto support OAuth popups.This description was created by
for cdc34d3. It will automatically update as commits are pushed.