Skip to content

fix(auth): update auth to use localStorage#543

Merged
domoritz merged 25 commits into
masterfrom
new-backend
Jun 10, 2025
Merged

fix(auth): update auth to use localStorage#543
domoritz merged 25 commits into
masterfrom
new-backend

Conversation

@domoritz

@domoritz domoritz commented Jun 8, 2025

Copy link
Copy Markdown
Member

No description provided.

@vercel

vercel Bot commented Jun 8, 2025

Copy link
Copy Markdown

Deployment failed with the following error:

Environment Variable "GITHUB_CLIENT_ID" references Secret "github-client-id", which does not exist.

@domoritz domoritz changed the title New backend fix(auth): update auth to use localStorage Jun 8, 2025
@domoritz

domoritz commented Jun 8, 2025

Copy link
Copy Markdown
Member Author

http://localhost:3000/auth/github/logout is leading to a loop

@domoritz domoritz requested a review from Copilot June 8, 2025 19:33

This comment was marked as outdated.

domoritz and others added 2 commits June 8, 2025 15:40
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment thread src/controllers/auth.ts Outdated
* @param {Response} res Response object
*/
private success = (req, res) => {
// eslint-disable-next-line no-console

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the log? I'd be okay with having console.info if it's really helpful

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was just leaving it until we tested the entire auth flow and then remove them once and for all. But I can remove them

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay. Let me know when it works so I can test it with the editor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can try testing now. Works for me locally

@domoritz

domoritz commented Jun 8, 2025

Copy link
Copy Markdown
Member Author

One thing that's a bit annoying right now is that http://localhost:3000/auth/github/check will return false if you open it in a stand alone window because we don't send the right token from local storage. Should we add another endpoint that sets the correct header?

@openhands-ai

openhands-ai Bot commented Jun 8, 2025

Copy link
Copy Markdown

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Test

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #543

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

@domoritz

domoritz commented Jun 8, 2025

Copy link
Copy Markdown
Member Author

I added debugging to the main endpoint. Note that the redirect doesn't work for debug yet. It should really redirect back to the main page but I get a loop.

@domoritz

Copy link
Copy Markdown
Member Author

When I go to http://localhost:3000 and do the auth, I should get redirected to http://localhost:3000, not the editor, though. Can you fix that?

@domoritz

Copy link
Copy Markdown
Member Author

Also, always make sure that listing passes. Just helps keep things clean.

@yuvvantalreja

Copy link
Copy Markdown
Member

When I go to http://localhost:3000 and do the auth, I should get redirected to http://localhost:3000, not the editor, though. Can you fix that?

Sure, I can change that.

@domoritz domoritz requested a review from Copilot June 10, 2025 19:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces session-based authentication (using Redis and express-session) with a token-based approach stored in localStorage, updates routing and controllers accordingly, and removes legacy functionality related to gists.

  • Migrate to token-based auth: generate/validate HMAC-signed tokens, add getGithubToken endpoint
  • Remove Redis/session middleware and related environment vars
  • Update Pug view to display and verify auth tokens

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vercel.json Removed Redis env vars
typedoc.json Switched to entryPoints schema, added $schema field
src/views/index.pug Added debug panel for token display and auth check script
src/urls.ts Added getGithubToken, removed Gist URLs, adjusted hosts
src/controllers/auth.ts Implemented token logic, removed session handling
src/app.ts Dropped session/Redis setup, switched to ES modules
package.json Set "type": "module", bumped deps
.vscode/settings.json Tweaked eslint code actions setting
Comments suppressed due to low confidence (3)

src/controllers/auth.ts:187

  • [nitpick] On logout you post {type: 'auth'} without a distinct flag for logout. Consider using a separate message type (e.g. {type: 'logout'}) so the opener can differentiate login vs. logout events.
{type: 'auth'}, '*'

typedoc.json:6

  • Trailing comma in a JSON array is invalid; remove the comma after the last entry to ensure typedoc.json parses correctly.
    "src/**/*.ts",

.vscode/settings.json:12

  • The source.fixAll.eslint setting expects a boolean, not a string. Change it to true or false to restore auto-fix on save.
"source.fixAll.eslint": "explicit"

Comment thread src/controllers/auth.ts
name: user._json.name,
profilePicUrl: user._json.avatar_url,
authToken: tokenUser ? authToken : this.generateToken(user),
githubAccessToken: tokenUser.accessToken,

Copilot AI Jun 10, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When tokenUser is null (session-based flow), this will throw. Use user.accessToken instead of tokenUser.accessToken to cover both cases.

Suggested change
githubAccessToken: tokenUser.accessToken,
githubAccessToken: user.accessToken,

Copilot uses AI. Check for mistakes.
Comment thread src/urls.ts
redirectUrl.successful = authUrl.isAuthenticated;
} else if (nodeEnv === 'development' || !nodeEnv) {
redirectUrl.successful = 'http://localhost:1234';
hostUrl = 'http://localhost:3000';

Copilot AI Jun 10, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nodeEnv has another value (e.g., staging), neither branch runs and hostUrl remains undefined. Consider adding a default case to avoid unexpected undefined values.

Suggested change
hostUrl = 'http://localhost:3000';
hostUrl = 'http://localhost:3000';
} else {
// Default case for unexpected `nodeEnv` values
redirectUrl.successful = 'https://default-url.com';
hostUrl = 'https://default-host.com';

Copilot uses AI. Check for mistakes.
@domoritz domoritz merged commit a6bbbd6 into master Jun 10, 2025
1 of 2 checks passed
@domoritz domoritz deleted the new-backend branch June 10, 2025 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants