Skip to content

Conversation

@rvyhnal
Copy link
Contributor

@rvyhnal rvyhnal commented Sep 24, 2024

This PR is taking out the login modal from base.mako into a new template login.mako. The redirect happens as part of the authenticatedAjax function.

#295

Comment on lines 578 to 587
function requireLogin(destinationUrl){
let currentPath = window.location.pathname.substring(1)
let loginUrl = "/login.html"

if ( ! ["login.html", "index.html"].includes(currentPath)){
loginUrl += "?r=" + encodeURIComponent(destinationUrl || currentPath)
}
window.location.href = loginUrl
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this function won't work in multiple cases. For example, IntelMQ manager in all instances I manage is deployed under a path /intelmq-manager. In addition, the main page is accessible directly under the path, without redirecting to index.html.

I've checked how the code would behave on the main page on one of my instances (in the browser's console):

> let currentPath = window.location.pathname.substring(1) 
> let loginUrl = "/login.html"
> currentPath
"intelmq-manager/"
> window.location.href = loginUrl
# Redirects to the main domain, not under the manager's path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the redirects so that all work with relative paths


let url = new URL(window.location.href)
let redirect = new URLSearchParams(url.search).get('r')
window.location.replace(redirect || "/index.html");
Copy link
Contributor

Choose a reason for hiding this comment

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

As a minimum precaution, we should verify the redirect target is related to the current path, to avoid an open redirect issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was assuming this wouldn't be an issue as the redirect happens only after the user is authenticated, anyways I have made a slight change, so that the redirect target is treated as relative path to the current location.

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.

2 participants