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

possible #1615 solution: partial wildcard allowed-origins, with just … #1623

Closed
wants to merge 1 commit into from

Conversation

Aterocana
Copy link
Contributor

…protocol specified (for example "http://*") could use glob ** pattern in order to match any sequence of characters (even the separator).

Signed-off-by: Aterocana <dominicimauriziogmail.com>

Related issue

@aeneasr as discussed in #1615 I'm proposing this fix.

Proposed changes

Basically I think the issue here is the protocol is specified, but no domain is: for example https://*.
Internally glob.Glob is compiled (with glob.Compile) with the provided string and '.' rune separator. Since g:=glob.Compile("https://*", '.') doesn't match g.Match("http://foobar.com") due to . separator, we could use g:=glob.Compile("https://**", '.') to ignore .` separator in this specific case.
In order to do so I simply tried to match if "://" substring is into an allowed-origin string.

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

…st protocol specified (for example "http://*") could use glob ** pattern in order to match any sequence of characters (even the separator).

Signed-off-by: Aterocana <dominicimaurizio<at>gmail.com>
@CLAassistant
Copy link

CLAassistant commented Oct 31, 2019

CLA assistant check
All committers have signed the CLA.

@aeneasr
Copy link
Member

aeneasr commented Oct 31, 2019

Awesome, thank you! Once the CLA is signed I'll merge this. Please comment once it's signed so I don't miss it!

@Aterocana
Copy link
Contributor Author

Yeah, It seems I used a different mail from GitHub's one. Let me check tomorrow, if necessary I'll do a different pr.

@Aterocana
Copy link
Contributor Author

I actually signed it, but it says I don't. Is it normal?

@aeneasr
Copy link
Member

aeneasr commented Nov 1, 2019

Hm, it says:

Maurizio Dominici seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

It also appears that your commit is not actually attributed to your GitHub account, as indicated by the greyed-out image:

image

To see how to link the local git commit to GitHub, here's a guide: https://help.github.com/en/github/setting-up-and-managing-your-github-user-account/setting-your-commit-email-address

Once the commit has the right metadata the toll should work :) Thank you!!

@Aterocana
Copy link
Contributor Author

I'm closing this since I recreated the PR due to git user used. Sorry.

@Aterocana Aterocana closed this Nov 1, 2019
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