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

PageSpeed URL control wildcards don't work properly #1294

Closed
crowell opened this issue Apr 26, 2016 · 9 comments
Closed

PageSpeed URL control wildcards don't work properly #1294

crowell opened this issue Apr 26, 2016 · 9 comments

Comments

@crowell
Copy link
Contributor

@crowell crowell commented Apr 26, 2016

In one level of .htaccess specify ModPagespeedDisallow * and in a higher level specify ModPagespeedAllow *example.com*, example.com will still be shown as disallowed.

@crowell crowell changed the title PageSpeed URL control interactions are broken PageSpeed URL control interactions don't merge properly Apr 26, 2016
@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Apr 26, 2016

If I understand your description, this is working as intended.

E.g.
htdocs/foo/.htaccess : ModPagespeedAllow *example.com*
htdocs/foo/bar/.htaccess : ModPagespeedDisallow *

Any time a file in foo/bar/ is referenced, the Disallow * will override the .htaccess in foo/

@crowell
Copy link
Contributor Author

@crowell crowell commented Apr 26, 2016

@jmarantz no, the opposite, the disallow in the root, and then the allow in
/foo/.htaccess
On Apr 26, 2016 11:44 AM, "Joshua Marantz" notifications@github.com wrote:

If I understand your description, this is working as intended.

E.g.
htdocs/foo/.htaccess : ModPagespeedAllow example.com
http://example.com

htdocs/foo/bar/.htaccess : ModPagespeedDisallow *

Any time a file in foo/bar/ is referenced, the Disallow * will override
the .htaccess in foo/


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1294 (comment)

jmarantz added a commit that referenced this issue Apr 27, 2016
#1294

The unit test works properly, so there is no code change in this commit.
The bug remains outstanding.
@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Jun 28, 2016

@jmarantz I'm having trouble understanding 07c1fcc: is it that you thought this might have been a bug in MergeOptions, and you created a unit test to elicit the bug, but the unit test passed?

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Jun 28, 2016

Reproduced:

mod_pagespeed_test/
  issue1294/
    .htaccess: ModPagespeedDisallow *
    subdir/
      .htaccess: ModPagespeedAllow *foo*
      foo.js: document.write("hello ");
      bar.js: document.write("world");
      index.html: <script src=foo.js></script><script src=bar.js></script>

$ curl localhost:8080/mod_pagespeed_test/issue1294/subdir/
<script src="foo.js"></script>
<script src="bar.js"></script>
@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Jun 28, 2016

When reversing the two .htaccess files we still don't see foo.js get rewritten, so it's not just handling them backwards or something.

@hillsp
Copy link
Contributor

@hillsp hillsp commented Jun 30, 2016

I cannot reproduce this using what Josh checked in above; ModPagespeedAllow *purple* still works just fine. If I comment out the entire Allow in the override dir, the test fails.

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Jun 30, 2016

The test @hillsp was referring to as "what Josh checked in above" was 4d3acd9 plus 6b0cd0c

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Jun 30, 2016

Hypothesis is that .htaccess merging works correctly in the ipro flow but not the html flow.

@hillsp hillsp changed the title PageSpeed URL control interactions don't merge properly PageSpeed URL control wildcards don't work properly Jul 7, 2016
@hillsp
Copy link
Contributor

@hillsp hillsp commented Jul 7, 2016

Turns out that this is a problem with FastWildcardGroup and isn't related to the merging at all. @jmaessen has a fix.

jmaessen added a commit that referenced this issue Jul 8, 2016
…e* order,

and should match them forwards as a result.  Add a couple of tests beyond
Steve's initial repro.

#1294
@jmaessen jmaessen closed this Jul 8, 2016
crowell added a commit that referenced this issue Jul 21, 2016
…e* order,

and should match them forwards as a result.  Add a couple of tests beyond
Steve's initial repro.

#1294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants