Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: fix post policy escape bug, update conformance tests #924

Merged
merged 2 commits into from
Aug 20, 2021
Merged

feat: fix post policy escape bug, update conformance tests #924

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8041f88
feat: fix post policy escape bug, update conformance tests
JesseLovelace Jul 20, 2021
5c981e2
fix typo
JesseLovelace Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,13 +443,21 @@ public String toJson() {
StringBuilder escapedJson = new StringBuilder();

// Certain characters in a policy must be escaped
for (char c : json.toCharArray()) {
char[] jsonArray = json.toCharArray();
for (int i = 0; i < jsonArray.length; i++) {
char c = jsonArray[i];
if (c >= 128) { // is a unicode character
escapedJson.append(String.format("\\u%04x", (int) c));
} else {
switch (c) {
case '\\':
escapedJson.append("\\\\");
// The JsonObject/JsonArray operations above handle quote escapes, so leave any "/""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is forward slash meant here? Looks like it should be a backslash

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be, will fix

// found alone
if (jsonArray[i + 1] == '"') {
escapedJson.append("\\");
} else {
escapedJson.append("\\\\");
}
break;
case '\b':
escapedJson.append("\\b");
Expand Down
25 changes: 11 additions & 14 deletions google-cloud-storage/src/test/java/com/google/cloud/storage/V4PostPolicyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import com.google.cloud.conformance.storage.v1.UrlStyle;
import com.google.cloud.storage.testing.RemoteStorageHelper;
import com.google.common.base.Charsets;
import com.google.common.base.Strings;
import com.google.common.io.BaseEncoding;
import com.google.protobuf.Timestamp;
import com.google.protobuf.util.JsonFormat;
Expand Down Expand Up @@ -117,17 +116,6 @@ public void test() {

PolicyConditions conditions = policyInput.getConditions();

if (!Strings.isNullOrEmpty(fields.get("success_action_redirect"))) {
builder.addSuccessActionRedirectUrlCondition(
PostPolicyV4.ConditionV4Type.MATCHES, fields.get("success_action_redirect"));
}

if (!Strings.isNullOrEmpty(fields.get("success_action_status"))) {
builder.addSuccessActionStatusCondition(
PostPolicyV4.ConditionV4Type.MATCHES,
Integer.parseInt(fields.get("success_action_status")));
}

if (conditions != null) {
if (!conditions.getStartsWithList().isEmpty()) {
builder.addCustomCondition(
Expand Down Expand Up @@ -166,17 +154,25 @@ public void test() {
style);

String expectedPolicy = testData.getPolicyOutput().getExpectedDecodedPolicy();

StringBuilder escapedPolicy = new StringBuilder();

// Java automatically unescapes the unicode escapes in the conformance tests, so we need to
// manually re-escape them
for (char c : expectedPolicy.toCharArray()) {
char[] expectedPolicyArray = expectedPolicy.toCharArray();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be reproducing the algorithm in the test. That risks copying bugs from one place to another, and verifying the buggy behavior. Can you make the expected output a literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they both happen to need to do the same thing (escaping certain characters). Sorry, I'm unsure what you mean by "make it a literal," can you elaborate?

for (int i = 0; i < expectedPolicyArray.length; i++) {
char c = expectedPolicyArray[i];
if (c >= 128) {
escapedPolicy.append(String.format("\\u%04x", (int) c));
} else {
switch (c) {
case '\\':
escapedPolicy.append("\\\\");
// quotes aren't unescaped, so leave any "\"" found alone
if (expectedPolicyArray[i + 1] == '"') {
escapedPolicy.append("\\");
} else {
escapedPolicy.append("\\\\");
}
break;
case '\b':
escapedPolicy.append("\\b");
Expand All @@ -202,6 +198,7 @@ public void test() {
}
}
assertEquals(testData.getPolicyOutput().getFieldsMap(), policy.getFields());

assertEquals(
escapedPolicy.toString(),
new String(BaseEncoding.base64().decode(policy.getFields().get("policy"))));
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-conformance-tests</artifactId>
<version>0.0.11</version>
<version>0.1.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this verify the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in fact the failures caused by it is what brought the bug to my attention (#510)

<scope>test</scope>
</dependency>
<dependency>
Expand Down