Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Prevents line breaks in delivery service remapText field (UI and API)#4305

Merged
ocket8888 merged 6 commits intoapache:masterfrom
mitchell852:issue-4298-line-break-remap-text
Jan 21, 2020
Merged

Prevents line breaks in delivery service remapText field (UI and API)#4305
ocket8888 merged 6 commits intoapache:masterfrom
mitchell852:issue-4298-line-break-remap-text

Conversation

@mitchell852
Copy link
Copy Markdown
Member

@mitchell852 mitchell852 commented Jan 17, 2020

What does this PR (Pull Request) do?

Rejects delivery service remapText with line breaks as it will result in a malformed remap.config file.

Which Traffic Control components are affected by this PR?

  • Traffic Ops
  • Traffic Portal

What is the best way to verify this PR?

  1. Run the UI tests
  2. Run the API tests
  3. Manually verify in TP by trying to enter a line break in the Raw Remap Text delivery service form field for a DNS, HTTP or ANY_MAP delivery service.
  4. Attempt to create or update a delivery service using the API where remapText contains \n or \r. It should fail.

If this is a bug fix, what versions of Traffic Control are affected?

  • master (6b11bd9)
  • 3.1.0
  • 4.0.0 (RC1)

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR ensures that database migration sequence is correct OR this PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

@mitchell852 mitchell852 added bug something isn't working as intended Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1 labels Jan 17, 2020
@ocket8888 ocket8888 self-assigned this Jan 17, 2020
@mitchell852 mitchell852 force-pushed the issue-4298-line-break-remap-text branch from d7cb9c0 to 1c9d89c Compare January 17, 2020 19:50
Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

The regular expression you're using isn't multi-line; ^ and $ match the beginning and end of a line, respectively. So I can still do this:

>> temp0.name
<- "remapText"
>> temp0.tagName
<- "TEXTAREA"
>> temp0.attributes[4]
<- ng-pattern="/^[^\\n\\r]*$/"
>> temp0.value
<- "
this is a test
"
>> temp0.validity.patternMismatch
<- false
>> temp0.classList.contains("ng-invalid")
<- false
>> temp0.classList.contains("ng-valid")
<- true

@mitchell852
Copy link
Copy Markdown
Member Author

mitchell852 commented Jan 17, 2020

The regular expression you're using isn't multi-line; ^ and $ match the beginning and end of a line, respectively. So I can still do this:

>> temp0.name
<- "remapText"
>> temp0.tagName
<- "TEXTAREA"
>> temp0.attributes[4]
<- ng-pattern="/^[^\\n\\r]*$/"
>> temp0.value
<- "
this is a test
"
>> temp0.validity.patternMismatch
<- false
>> temp0.classList.contains("ng-invalid")
<- false
>> temp0.classList.contains("ng-valid")
<- true

well in the UI i am unable to enter anything on 2+ lines so that's what i was going for. for example

image

^^ is fine as it appears to get trimmed

image

2 lines with content isn't allowed.

@ocket8888
Copy link
Copy Markdown
Contributor

No, you still can.

Depiction of the problem

That'll get rejected by the API but the form shows no problems. According to the issue, the content of this field is:

"appended to the one line [in remap.config for most Delivery Services]"

so this value would still exhibit the problem. So it ought to not be considered correct - especially since submission will result in an error.

@mitchell852
Copy link
Copy Markdown
Member Author

mitchell852 commented Jan 21, 2020

so this value would still exhibit the problem. So it ought to not be considered correct - especially since submission will result in an error.

hmm. i can't replicate. this submits fine in both chrome and firefox as i would hope.

image

it makes it past the api so it appears to be getting trimmed. also, remap.config looks fine when i submit that value ^^

it's only when i added a value on a 2nd line that the field becomes invalid which is what i want.

image

@mitchell852
Copy link
Copy Markdown
Member Author

retest this please

Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

API/client integration tests pass (unrelated failure in TestStatsSummary notwithstanding), unit tests pass, you didn't add a TP UI test so I assume those all still pass (though honestly that'd be nice), manual requests show newlines rejected by API and for some reason I can't explain the ng-model bindings on textareas silently trim leading and trailing whitespace so that isn't really a problem either.

Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Actually, after running the tests again against this branch rebased on master and master itself, TestStatsSummary is failing only after this branch's changes. I have no idea how or why, but that seems to be the case.

Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Nevermind again, that was just a transient, inexplicable, temporary failure :))))))))))))))))))))

@ocket8888 ocket8888 merged commit c025d22 into apache:master Jan 21, 2020
mitchell852 added a commit to mitchell852/trafficcontrol that referenced this pull request Jan 22, 2020
…apache#4305)

* prevents line breaks in ds.remapText (UI and API)

* adds test to ensure ds.remapText cannot include a line break

* updated fixtures to include valid line breaks for edge/mid header rewrites, regex remap and tr request/response headers

* adds period to GoDoc

* no need for ng-pattern

* textarea does not support pattern so switching back to ng-pattern

(cherry picked from commit c025d22)
rawlinp pushed a commit that referenced this pull request Jan 22, 2020
…#4305) (#4317)

* prevents line breaks in ds.remapText (UI and API)

* adds test to ensure ds.remapText cannot include a line break

* updated fixtures to include valid line breaks for edge/mid header rewrites, regex remap and tr request/response headers

* adds period to GoDoc

* no need for ng-pattern

* textarea does not support pattern so switching back to ng-pattern

(cherry picked from commit c025d22)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug something isn't working as intended Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Line breaks in DS raw remap text will result in a malformed remap.config file

3 participants