You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2017/12/04 16:59:13 UTC
svn commit: r1817105 - in /tomcat/trunk:
java/org/apache/catalina/core/ApplicationPushBuilder.java
webapps/docs/changelog.xml
Author: remm
Date: Mon Dec 4 16:59:12 2017
New Revision: 1817105
URL: http://svn.apache.org/viewvc?rev=1817105&view=rev
Log:
Minor push builder fixes: don't remove the auth header, and exception on an empty method.
Modified:
tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java?rev=1817105&r1=1817104&r2=1817105&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java Mon Dec 4 16:59:12 2017
@@ -98,7 +98,6 @@ public class ApplicationPushBuilder impl
headers.remove("if-range");
headers.remove("range");
headers.remove("expect");
- headers.remove("authorization");
headers.remove("referer");
// Also remove the cookie header since it will be regenerated
headers.remove("cookie");
@@ -108,7 +107,6 @@ public class ApplicationPushBuilder impl
if (request.getQueryString() != null) {
referer.append('?');
referer.append(request.getQueryString());
-
}
addHeader("referer", referer.toString());
@@ -184,7 +182,7 @@ public class ApplicationPushBuilder impl
@Override
public PushBuilder method(String method) {
String upperMethod = method.trim().toUpperCase();
- if (DISALLOWED_METHODS.contains(upperMethod)) {
+ if (DISALLOWED_METHODS.contains(upperMethod) || upperMethod.length() == 0) {
throw new IllegalArgumentException(
sm.getString("applicationPushBuilder.methodInvalid", upperMethod));
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1817105&r1=1817104&r2=1817105&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec 4 16:59:12 2017
@@ -53,6 +53,9 @@
<fix>
Update the Java EE 8 XML schema to the released versions. (markt)
</fix>
+ <fix>
+ Minor HTTP/2 push fixes. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Dec 4, 2017 at 9:22 PM, Mark Thomas <ma...@apache.org> wrote:
> On 04/12/17 19:50, Mark Thomas wrote:
> > On 04/12/17 18:03, Rémy Maucherat wrote:
>
> <snip/>
>
> >> Another "feature" that looks almost impossible to implement I guess.
> >
> > Hmm. I only read the first part of the Javadoc. I'm not really sure what
> > the second part is getting at with "... a container generated token...".
> > I'll have a look back at the archive to see if there was any EG
> > discussion on this point.
>
> That second part was part of the original proposal and there was never
> any discussion about what it actually meant.
>
> Thinking about it, I think we could do the following and be spec compliant:
>
> - Set a header e.g. "Authorization: x-push"
> - Copy the authenticated Principal from the base request to the
> pushTarget
>
> That meets the requirements:
> - "an Authorization header will be set with a container generated token"
> - "result in equivalent Authorization for the pushed request"
>
> The spec does imply that it is the token that results in authorization
> but it doesn't actually mandate it. I think there is enough flexibility
> in the wording that the above would be OK.
>
> Thoguhts?
>
> Indeed, it doesn't say that it has to be an autorization header that would
normally work, only a token.
Rémy
Re: svn commit: r1817105 - in /tomcat/trunk:
java/org/apache/catalina/core/ApplicationPushBuilder.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 04/12/17 19:50, Mark Thomas wrote:
> On 04/12/17 18:03, Rémy Maucherat wrote:
<snip/>
>> Another "feature" that looks almost impossible to implement I guess.
>
> Hmm. I only read the first part of the Javadoc. I'm not really sure what
> the second part is getting at with "... a container generated token...".
> I'll have a look back at the archive to see if there was any EG
> discussion on this point.
That second part was part of the original proposal and there was never
any discussion about what it actually meant.
Thinking about it, I think we could do the following and be spec compliant:
- Set a header e.g. "Authorization: x-push"
- Copy the authenticated Principal from the base request to the
pushTarget
That meets the requirements:
- "an Authorization header will be set with a container generated token"
- "result in equivalent Authorization for the pushed request"
The spec does imply that it is the token that results in authorization
but it doesn't actually mandate it. I think there is enough flexibility
in the wording that the above would be OK.
Thoguhts?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1817105 - in /tomcat/trunk:
java/org/apache/catalina/core/ApplicationPushBuilder.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 04/12/17 18:03, Rémy Maucherat wrote:
> On Mon, Dec 4, 2017 at 6:05 PM, Mark Thomas <ma...@apache.org> wrote:
>
>> On 04/12/17 16:59, remm@apache.org wrote:
>>> Author: remm
>>> Date: Mon Dec 4 16:59:12 2017
>>> New Revision: 1817105
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1817105&view=rev
>>> Log:
>>> Minor push builder fixes: don't remove the auth header,
>>
>> -1.
>>
>> The Javadoc for PushBuilder explicitly lists Authorization headers as
>> one of the types that are not transferred to the pushed request.
>>
>
> And then:
> If the request was authenticated, an Authorization header will be set with
> a container generated token that will result in equivalent Authorization
> for the pushed request.
>
> So it worked just fine for basic.
>
> Another "feature" that looks almost impossible to implement I guess.
Hmm. I only read the first part of the Javadoc. I'm not really sure what
the second part is getting at with "... a container generated token...".
I'll have a look back at the archive to see if there was any EG
discussion on this point.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1817105 - in /tomcat/trunk: java/org/apache/catalina/core/ApplicationPushBuilder.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Dec 4, 2017 at 6:05 PM, Mark Thomas <ma...@apache.org> wrote:
> On 04/12/17 16:59, remm@apache.org wrote:
> > Author: remm
> > Date: Mon Dec 4 16:59:12 2017
> > New Revision: 1817105
> >
> > URL: http://svn.apache.org/viewvc?rev=1817105&view=rev
> > Log:
> > Minor push builder fixes: don't remove the auth header,
>
> -1.
>
> The Javadoc for PushBuilder explicitly lists Authorization headers as
> one of the types that are not transferred to the pushed request.
>
And then:
If the request was authenticated, an Authorization header will be set with
a container generated token that will result in equivalent Authorization
for the pushed request.
So it worked just fine for basic.
Another "feature" that looks almost impossible to implement I guess.
Rémy
> > and exception on an empty method.
>
> Good catch.
>
> Mark
>
> [1]
> https://github.com/javaee/servlet-spec/blob/master/src/
> main/java/javax/servlet/http/PushBuilder.java
>
>
>
> >
> > Modified:
> > tomcat/trunk/java/org/apache/catalina/core/
> ApplicationPushBuilder.java
> > tomcat/trunk/webapps/docs/changelog.xml
> >
> > Modified: tomcat/trunk/java/org/apache/catalina/core/
> ApplicationPushBuilder.java
> > URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/
> catalina/core/ApplicationPushBuilder.java?rev=1817105&r1=1817104&r2=
> 1817105&view=diff
> > ============================================================
> ==================
> > --- tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> (original)
> > +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> Mon Dec 4 16:59:12 2017
> > @@ -98,7 +98,6 @@ public class ApplicationPushBuilder impl
> > headers.remove("if-range");
> > headers.remove("range");
> > headers.remove("expect");
> > - headers.remove("authorization");
> > headers.remove("referer");
> > // Also remove the cookie header since it will be regenerated
> > headers.remove("cookie");
> > @@ -108,7 +107,6 @@ public class ApplicationPushBuilder impl
> > if (request.getQueryString() != null) {
> > referer.append('?');
> > referer.append(request.getQueryString());
> > -
> > }
> > addHeader("referer", referer.toString());
> >
> > @@ -184,7 +182,7 @@ public class ApplicationPushBuilder impl
> > @Override
> > public PushBuilder method(String method) {
> > String upperMethod = method.trim().toUpperCase();
> > - if (DISALLOWED_METHODS.contains(upperMethod)) {
> > + if (DISALLOWED_METHODS.contains(upperMethod) ||
> upperMethod.length() == 0) {
> > throw new IllegalArgumentException(
> > sm.getString("applicationPushBuilder.methodInvalid",
> upperMethod));
> > }
> >
> > Modified: tomcat/trunk/webapps/docs/changelog.xml
> > URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/
> changelog.xml?rev=1817105&r1=1817104&r2=1817105&view=diff
> > ============================================================
> ==================
> > --- tomcat/trunk/webapps/docs/changelog.xml (original)
> > +++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec 4 16:59:12 2017
> > @@ -53,6 +53,9 @@
> > <fix>
> > Update the Java EE 8 XML schema to the released versions.
> (markt)
> > </fix>
> > + <fix>
> > + Minor HTTP/2 push fixes. (remm)
> > + </fix>
> > </changelog>
> > </subsection>
> > <subsection name="Coyote">
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: svn commit: r1817105 - in /tomcat/trunk:
java/org/apache/catalina/core/ApplicationPushBuilder.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 04/12/17 16:59, remm@apache.org wrote:
> Author: remm
> Date: Mon Dec 4 16:59:12 2017
> New Revision: 1817105
>
> URL: http://svn.apache.org/viewvc?rev=1817105&view=rev
> Log:
> Minor push builder fixes: don't remove the auth header,
-1.
The Javadoc for PushBuilder explicitly lists Authorization headers as
one of the types that are not transferred to the pushed request.
> and exception on an empty method.
Good catch.
Mark
[1]
https://github.com/javaee/servlet-spec/blob/master/src/main/java/javax/servlet/http/PushBuilder.java
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java?rev=1817105&r1=1817104&r2=1817105&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/core/ApplicationPushBuilder.java Mon Dec 4 16:59:12 2017
> @@ -98,7 +98,6 @@ public class ApplicationPushBuilder impl
> headers.remove("if-range");
> headers.remove("range");
> headers.remove("expect");
> - headers.remove("authorization");
> headers.remove("referer");
> // Also remove the cookie header since it will be regenerated
> headers.remove("cookie");
> @@ -108,7 +107,6 @@ public class ApplicationPushBuilder impl
> if (request.getQueryString() != null) {
> referer.append('?');
> referer.append(request.getQueryString());
> -
> }
> addHeader("referer", referer.toString());
>
> @@ -184,7 +182,7 @@ public class ApplicationPushBuilder impl
> @Override
> public PushBuilder method(String method) {
> String upperMethod = method.trim().toUpperCase();
> - if (DISALLOWED_METHODS.contains(upperMethod)) {
> + if (DISALLOWED_METHODS.contains(upperMethod) || upperMethod.length() == 0) {
> throw new IllegalArgumentException(
> sm.getString("applicationPushBuilder.methodInvalid", upperMethod));
> }
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1817105&r1=1817104&r2=1817105&view=diff
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Mon Dec 4 16:59:12 2017
> @@ -53,6 +53,9 @@
> <fix>
> Update the Java EE 8 XML schema to the released versions. (markt)
> </fix>
> + <fix>
> + Minor HTTP/2 push fixes. (remm)
> + </fix>
> </changelog>
> </subsection>
> <subsection name="Coyote">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org