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

MapProxyDomain does not work with InPlaceResourceOptimization #1015

Closed
jmarantz opened this Issue Oct 8, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@jmarantz
Copy link
Contributor

jmarantz commented Oct 8, 2015

I think this should be straightforward to support, as the MapProxyDomain prefixes are established at configuration time and can be statically checked in the handler at request-time.

Note that there are a variety of ipro/MapProxyDomain tests around https://github.com/pagespeed/mod_pagespeed/blob/master/pagespeed/apache/system_test.sh#L988 but those are not shared with nginx.

In the shared system_test script there is a test for MapProxyDomain from the HTML flow: https://github.com/pagespeed/mod_pagespeed/blob/master/pagespeed/system/system_test.sh#L241

but there is no evidence of testing in that shared file for MapProxyDomain of in-place resources.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 8, 2015

In normal usage this isn't a problem, because any resources PageSpeed moves onto the mapped proxied domain it will be able to create .pagespeed. resources for, but if you want to set up a server to do IPRO optimization of another server this would be a useful feature.

(This does work in Apache.)

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 8, 2015

Also, in general it's safer if we accept these, in case a CSS file we proxy onto our domain references something relative and we don't manage to rewrite it's url for some reason.

@jmarantz

This comment has been minimized.

Copy link
Contributor

jmarantz commented Oct 22, 2015

@oschaaf oschaaf self-assigned this Oct 26, 2015

oschaaf added a commit to apache/incubator-pagespeed-mod that referenced this issue Oct 27, 2015

nps-1015: IPRO+MPD support in ngx_pagespeed
- Move the Apache tests for IPRO + MPD to system/system_test.sh
- Add a flag 'trusted_input_' in ProxyFetch to allow ngx_pagespeed
  to transform html but not proxy external html fetched via MPD.

MPS-side of the fix for:
apache/incubator-pagespeed-ngx#1015
@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Oct 27, 2015

I created pull requests with changes to make this work for nps (note there's also an MPS side to this).
If the new 'trusted_input' flag in ProxyFetch is not a good idea and another approach would be better, I'd love to hear that.

oschaaf added a commit to apache/incubator-pagespeed-mod that referenced this issue Oct 27, 2015

nps-1015: IPRO+MPD support in ngx_pagespeed
- Move the Apache tests for IPRO + MPD to system/system_test.sh
- Add a flag 'trusted_input_' in ProxyFetch to allow ngx_pagespeed
  to transform html but not proxy external html fetched via MPD.

MPS-side of the fix for:
apache/incubator-pagespeed-ngx#1015

oschaaf added a commit to apache/incubator-pagespeed-mod that referenced this issue Oct 27, 2015

nps-1015: IPRO+MPD support in ngx_pagespeed
- Move the Apache tests for IPRO + MPD to system/system_test.sh
- Add a flag 'trusted_input_' in ProxyFetch to allow ngx_pagespeed
  to transform html but not proxy external html fetched via MPD.

MPS-side of the fix for:
apache/incubator-pagespeed-ngx#1015

oschaaf added a commit to apache/incubator-pagespeed-mod that referenced this issue Oct 27, 2015

nps-1015: IPRO+MPD support in ngx_pagespeed
- Move the Apache tests for IPRO + MPD to system/system_test.sh
- Add a flag 'trusted_input_' in ProxyFetch to allow ngx_pagespeed
  to transform html but not proxy external html fetched via MPD.

MPS-side of the fix for:
apache/incubator-pagespeed-ngx#1015
@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Oct 27, 2015

LGTM. I'll merge this along with the apache side of the change once I finish running internal tests.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Nov 30, 2015

Fixed with becc2f6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment