Skip to content
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.

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

remove trailing slash for overrideUrlName #216

Merged
merged 0 commits into from
Aug 17, 2022

Conversation

jordiprats
Copy link

@jordiprats jordiprats commented Feb 8, 2021

Hi,
This PR is for fixing what could be also considered as a configuration mistake. If you set overrideBaseUrl to an url with a trailing slash as follows:

        apiSecurity:
          overrideBaseUrl: http://whatever.com/
        uiSecurity:
          overrideBaseUrl: http://whatever.com/

It looks like everything is fine but if you click on the source link on any pipeline you'll get this error message:

The request was rejected because the URL contained a potentially malicious String

Taking a look at the URL you'll realize there's a double slash after the hostname which leads to this error. On my PR you'll find the code needed for ignoring this final slash: It doesn't even matter if you remove it or add the final slash: the operator won't even bother updating the containers because it's just ignored

kind regards,

@jordiprats
Copy link
Author

Regading the failed build looks like it's something unrelated to the code:


Run docker login -u  -p "" registry.redhat.io
"docker login" requires at most 1 argument.
See 'docker login --help'.

Usage:  docker login [OPTIONS] [SERVER]

Log in to a Docker registry
Error: Process completed with exit code 1.

@jordiprats jordiprats changed the title remore trailing slash for overrideUrlName remove trailing slash for overrideUrlName Feb 9, 2021
// ignore error, overrideBaseUrl may not be set in hal config
statusUrl, _ := t.svc.GetSpinnakerConfig().GetHalConfigPropString(ctx, overrideUrlName)
if overrideUrlName[len(overrideUrlName)-1:] == "/" {

Choose a reason for hiding this comment

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

You should leverage strings and do strings.HasPrefix(overrideUrlName, "/")

if strings.HasPrefix(overrideUrlName, "/") {
    overrideUrlName = overrideUrlName[:len(overrideUrlName) - 1]
}

You could drop the else and the additional variable.

@jordiprats
Copy link
Author

jordiprats commented Mar 18, 2021

Good point, although it is actually HasSuffix: tests whether the string s ends with suffix.

I have pushed an amended commit for this

@jordiprats
Copy link
Author

This change is quite straightforward and it's going to help users avoid getting stuck with this issue. Can someone take a look?

@ichi0915
Copy link
Contributor

Thank you for this, I don't have permissions to merge this until the actions have passed let me see who can help us get this merge

@ichi0915
Copy link
Contributor

@jordiprats can you help us rebasing to the latest commits in master or maybe create an empty commit just to re-run the actions and see the output.

@jordiprats
Copy link
Author

@ichi0915 sorry I messed up with this PR, I have created a new one with the change rebased: #267

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