Skip to content

Allow login while proxing a sub-domain #2186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
code-asher wants to merge 1 commit into from
Closed

Allow login while proxing a sub-domain #2186

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/node/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,29 @@ export class HttpServer {
}

// Must be authenticated to use the proxy.
route.provider.ensureAuthenticated(request)
if (!route.provider.authenticated(request)) {
// Attempt to determine if it's the user browsing the root and if so fall

@nhooyr nhooyr Oct 12, 2020

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.

Suggested change
// Attempt to determine if it's the user browsing the root and if so fall
// Attempt to determine if it's the user browsing the route and if so fall

@code-asher code-asher Oct 12, 2020

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.

I mean root, for example 8080.mydomain.com and not 8080.mydomain.com/some/page. Maybe we should always redirect though even if it's not the root.

@code-asher code-asher Oct 12, 2020

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.

Well I guess we can't do that safely, it would end up causing the problem in #2128 again on account of the whole not being able to detect HTML nonsense.

// through to allow the login flow.
if (request.headers["content-type"] !== "application/json") {

@nhooyr nhooyr Oct 12, 2020

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.

Let's check explicitly for text/html here.

@code-asher code-asher Oct 12, 2020

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.

I wanted to do that but Firefox and Chromium don't seem to set this at all. When I logged it the value was undefined.

switch (route.providerBase) {

@nhooyr nhooyr Oct 12, 2020

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.

I'm confused by this, don't we pretty always want to fall through to the login flow if text/html is asked for but the user isn't authenticated?

@code-asher code-asher Oct 12, 2020

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.

Yup, if there was a way to reliably detect the request was asking for HTML (and ideally also that it wasn't an xhr request) that would 100% be the way to do it. All this jank is to work around not being able to do that (at least as far as I know, I hope I'm wrong though).

case "/":
case "/static":
if (request.method === "GET") {
return undefined
}
break
case "/login":
if (request.method === "GET" || request.method === "POST") {
return undefined
}
break
}
}

// Assume anything else is some kind of request from the proxied
// application and return an unauthorized message.
throw new HttpError("Unauthorized", HttpCode.Unauthorized)
}

return {
proxy: {
Expand Down