Skip to content

Commit

Permalink
Issue #5171 Simplify GzipHandler user-agent handling (#5196)
Browse files Browse the repository at this point in the history 
* Issue #5171 Simplify GzipHandler user-agent handling

+ Remove User-Agent handling from GzipHandler
+ Allow Vary header to be set
+ Create rewrite MsieRule to remove Accept-Encoding from IE<=6

Signed-off-by: Greg Wilkins <[email protected]>

* + Full implementation of HttpFields ensure
 + use for Vary field

* + fixed checkstyle

* + fixed test for merged header

* + fixed javadoc

* Issue #5171 Simplify GzipHandler user-agent handling

 + improved comments

Signed-off-by: Greg Wilkins <[email protected]>

* rename and testing after review
  • Loading branch information
@gregw
gregw authored Aug 26, 2020
1 parent 7abb9d3 commit 601710c
Show file tree
Hide file tree
Showing 16 changed files with 758 additions and 397 deletions.
153 changes: 153 additions & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -738,6 +738,159 @@ public Mutable clear()
return this;
}

/** Ensure that specific HttpField exists when the field may not exist or may
* exist and be multi valued. Multiple existing fields are merged into a
* single field.
* @param field The header to ensure is contained. The field is used
* directly if possible so {@link PreEncodedHttpField}s can be
* passed. If the value needs to be merged with existing values,
* then a new field is created.
*/
public void ensureField(HttpField field)
{
// Is the field value multi valued?
if (field.getValue().indexOf(',') < 0)
{
// Call Single valued computeEnsure with either String header name or enum HttpHeader
if (field.getHeader() != null)
computeField(field.getHeader(), (h, l) -> computeEnsure(field, l));
else
computeField(field.getName(), (h, l) -> computeEnsure(field, l));
}
else
{
// call multi valued computeEnsure with either String header name or enum HttpHeader
if (field.getHeader() != null)
computeField(field.getHeader(), (h, l) -> computeEnsure(field, field.getValues(), l));
else
computeField(field.getName(), (h, l) -> computeEnsure(field, field.getValues(), l));
}
}

/**
* Compute ensure field with a single value
* @param ensure The field to ensure exists
* @param fields The list of existing fields with the same header
*/
private static HttpField computeEnsure(HttpField ensure, List<HttpField> fields)
{
// If no existing fields return the ensure field
if (fields == null || fields.isEmpty())
return ensure;

String ensureValue = ensure.getValue();

// Handle a single existing field
if (fields.size() == 1)
{
// If the existing field contains the ensure value, return it, else append values.
HttpField f = fields.get(0);
return f.contains(ensureValue)
? f
: new HttpField(ensure.getHeader(), ensure.getName(), f.getValue() + ", " + ensureValue);
}

// Handle multiple existing fields
StringBuilder v = new StringBuilder();
for (HttpField f : fields)
{
// Always append multiple fields into a single field value
if (v.length() > 0)
v.append(", ");
v.append(f.getValue());

// check if the ensure value is already contained
if (ensureValue != null && f.contains(ensureValue))
ensureValue = null;
}

// If the ensure value was not contained append it
if (ensureValue != null)
v.append(", ").append(ensureValue);

return new HttpField(ensure.getHeader(), ensure.getName(), v.toString());
}

/**
* Compute ensure field with a multiple values
* @param ensure The field to ensure exists
* @param values The QuotedCSV parsed field values.
* @param fields The list of existing fields with the same header
*/
private static HttpField computeEnsure(HttpField ensure, String[] values, List<HttpField> fields)
{
// If no existing fields return the ensure field
if (fields == null || fields.isEmpty())
return ensure;

// Handle a single existing field
if (fields.size() == 1)
{
HttpField f = fields.get(0);
// check which ensured values are already contained
int ensured = values.length;
for (int i = 0; i < values.length; i++)
{
if (f.contains(values[i]))
{
ensured--;
values[i] = null;
}
}

// if all ensured values contained return the existing field
if (ensured == 0)
return f;
// else if no ensured values contained append the entire ensured valued
if (ensured == values.length)
return new HttpField(ensure.getHeader(), ensure.getName(),
f.getValue() + ", " + ensure.getValue());
// else append just the ensured values that are not contained
StringBuilder v = new StringBuilder(f.getValue());
for (String value : values)
{
if (value != null)
v.append(", ").append(value);
}
return new HttpField(ensure.getHeader(), ensure.getName(), v.toString());
}

// Handle a multiple existing field
StringBuilder v = new StringBuilder();
int ensured = values.length;
for (HttpField f : fields)
{
// Always append multiple fields into a single field value
if (v.length() > 0)
v.append(", ");
v.append(f.getValue());

// null out ensured values that are included
for (int i = 0; i < values.length; i++)
{
if (values[i] != null && f.contains(values[i]))
{
ensured--;
values[i] = null;
}
}
}

// if no ensured values exist append them all
if (ensured == values.length)
v.append(", ").append(ensure.getValue());
// else if some ensured values are missing, append them
else if (ensured > 0)
{
for (String value : values)
if (value != null)
v.append(", ").append(value);
}

// return a merged header with missing ensured values added
return new HttpField(ensure.getHeader(), ensure.getName(), v.toString());
}

@Override
public boolean equals(Object o)
{
Expand Down
140 changes: 138 additions & 2 deletions jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -899,7 +900,6 @@ public void testComputeField()
{
HttpFields.Mutable fields = HttpFields.build();
assertThat(fields.size(), is(0));

fields.computeField("Test", (n, f) -> null);
assertThat(fields.size(), is(0));

Expand All @@ -922,4 +922,140 @@ public void testComputeField()
fields.computeField("TEST", (n, f) -> null);
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Before: value", "After: value"));
}
}

@Test
public void testEnsureSingleValue()
{
HttpFields.Mutable fields = HttpFields.build();

// 0 existing case
assertThat(fields.size(), is(0));
fields.ensureField(new PreEncodedHttpField(HttpHeader.VARY, "one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one"));
assertThat(fields.getField(0), instanceOf(PreEncodedHttpField.class));

// 1 existing cases
fields.ensureField(new HttpField(HttpHeader.VARY, "one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one"));
;
fields.ensureField(new HttpField(HttpHeader.VARY, "two"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two"));

// many existing cases
fields.put(new HttpField(HttpHeader.VARY, "one"));
fields.add(new HttpField(HttpHeader.VARY, "two"));
fields.ensureField(new HttpField(HttpHeader.VARY, "one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two"));

fields.put(new HttpField(HttpHeader.VARY, "one"));
fields.add(new HttpField(HttpHeader.VARY, "two"));
fields.ensureField(new HttpField(HttpHeader.VARY, "three"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two, three"));
}

@Test
public void testEnsureMultiValue()
{
HttpFields.Mutable fields = HttpFields.build();

// zero existing case
assertThat(fields.size(), is(0));
fields.ensureField(new PreEncodedHttpField(HttpHeader.VARY, "one, two"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two"));
assertThat(fields.getField(0), instanceOf(PreEncodedHttpField.class));

// one existing cases
fields.ensureField(new HttpField(HttpHeader.VARY, "two, one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two"));

fields.ensureField(new HttpField(HttpHeader.VARY, "three, one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two, three"));

fields.ensureField(new HttpField(HttpHeader.VARY, "four, five"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two, three, four, five"));

// many existing cases
fields.put(new HttpField(HttpHeader.VARY, "one"));
fields.add(new HttpField(HttpHeader.VARY, "two"));
fields.ensureField(new HttpField(HttpHeader.VARY, "two, one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two"));

fields.put(new HttpField(HttpHeader.VARY, "one"));
fields.add(new HttpField(HttpHeader.VARY, "two"));
fields.ensureField(new HttpField(HttpHeader.VARY, "three, two"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two, three"));

fields.put(new HttpField(HttpHeader.VARY, "one"));
fields.add(new HttpField(HttpHeader.VARY, "two"));
fields.ensureField(new HttpField(HttpHeader.VARY, "three, four"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Vary: one, two, three, four"));
}

@Test
public void testEnsureStringSingleValue()
{
HttpFields.Mutable fields = HttpFields.build();

// 0 existing case
assertThat(fields.size(), is(0));
fields.ensureField(new PreEncodedHttpField("Test", "one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one"));
assertThat(fields.getField(0), instanceOf(PreEncodedHttpField.class));

// 1 existing cases
fields.ensureField(new HttpField("Test", "one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one"));
;
fields.ensureField(new HttpField("Test", "two"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two"));

// many existing cases
fields.put(new HttpField("Test", "one"));
fields.add(new HttpField("Test", "two"));
fields.ensureField(new HttpField("Test", "one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two"));

fields.put(new HttpField("Test", "one"));
fields.add(new HttpField("Test", "two"));
fields.ensureField(new HttpField("Test", "three"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two, three"));
}

@Test
public void testEnsureStringMultiValue()
{
HttpFields.Mutable fields = HttpFields.build();

// zero existing case
assertThat(fields.size(), is(0));
fields.ensureField(new PreEncodedHttpField("Test", "one, two"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two"));
assertThat(fields.getField(0), instanceOf(PreEncodedHttpField.class));

// one existing cases
fields.ensureField(new HttpField("Test", "two, one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two"));

fields.ensureField(new HttpField("Test", "three, one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two, three"));

fields.ensureField(new HttpField("Test", "four, five"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two, three, four, five"));

// many existing cases
fields.put(new HttpField("Test", "one"));
fields.add(new HttpField("Test", "two"));
fields.ensureField(new HttpField("Test", "two, one"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two"));

fields.put(new HttpField("Test", "one"));
fields.add(new HttpField("Test", "two"));
fields.ensureField(new HttpField("Test", "three, two"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two, three"));

fields.put(new HttpField("Test", "one"));
fields.add(new HttpField("Test", "two"));
fields.ensureField(new HttpField("Test", "three, four"));
assertThat(fields.stream().map(HttpField::toString).collect(Collectors.toList()), contains("Test: one, two, three, four"));
}
}
13 changes: 13 additions & 0 deletions jetty-rewrite/src/main/config/modules/msie.mod
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# DO NOT EDIT - See: https://www.eclipse.org/jetty/documentation/current/startup-modules.html

[description]
Enables the MSIE rewrite rule for MSIE 5 and 6 known bugs.

[depend]
rewrite

[files]
basehome:modules/rewrite/rewrite-msie.xml|etc/rewrite-msie.xml

[xml]
etc/rewrite-msie.xml
10 changes: 10 additions & 0 deletions jetty-rewrite/src/main/config/modules/rewrite/rewrite-msie.xml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0"?>
<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_10_0.dtd">
<Configure id="Rewrite" class="org.eclipse.jetty.rewrite.handler.RuleContainer">
<Call name="addRule">
<Arg>
<New class="org.eclipse.jetty.rewrite.handler.MsieRule"/>
</Arg>
</Call>
</Configure>

16 changes: 8 additions & 8 deletions jetty-rewrite/src/main/config/modules/rewrite/rewrite-rules.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,6 @@
<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_10_0.dtd">
<Configure id="Rewrite" class="org.eclipse.jetty.rewrite.handler.RuleContainer">

<!-- Add rule to protect against IE ssl bug
<Call name="addRule">
<Arg>
<New class="org.eclipse.jetty.rewrite.handler.MsieSslRule"/>
</Arg>
</Call>
-->

<!-- protect favicon handling
<Call name="addRule">
<Arg>
Expand Down Expand Up @@ -102,5 +94,13 @@
</Call>
-->

<!-- Add rule to protect against IE ssl bug (see msie module and rewrite-msie.xml)
<Call name="addRule">
<Arg>
<New class="org.eclipse.jetty.rewrite.handler.MsieRule"/>
</Arg>
</Call>
-->

</Configure>

Loading

0 comments on commit 601710c

Please sign in to comment.