Add copy to clipboard buttons to display token page#200
Conversation
vojtechszocs
force-pushed
the
token-page-copy-to-clipboard
branch
from
0f2e803 to
f02b3b4
Compare
vojtechszocs
force-pushed
the
token-page-copy-to-clipboard
branch
2 times, most recently
from
3632827 to
3cd1e22
Compare
vojtechszocs
force-pushed
the
token-page-copy-to-clipboard
branch
from
3cd1e22 to
c9a96d7
Compare
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
This seems fine to me. I don't think I have approval permissions. Will likely need @ibihim for approval.
/lgtm
| <code>{{.AccessToken}}</code> | ||
|
|
||
| <script> | ||
| var commands = { |
There was a problem hiding this comment.
Maybe this is my old school JS, but isn't it better to have something like this namespaced approach:
var openshift = openshift || {};
openshift.commands = openshift.commands || {};
openshift.commands.login = '...';
openshift.commands.apiCall = '...';or a IIFE that is completely isolated?
There was a problem hiding this comment.
Before this PR, the token request page had no JavaScript so I'm not sure if using the openshift variable to namespace the commands is really needed.
I wanted to keep the changes at minimum so I went with a single commands variable to hold the strings to copy.
var commands = { foo: 'x' };is functionally the same as
var commands = {};
commands.foo = 'x';As for IIFE, these are typically used to avoid pollution of the global object namespace but I don't think we need them in our case.
The idea is to define some global object to store the strings to be referenced by the copy button's onclick handler.
There was a problem hiding this comment.
I still think that it is always worth to introduce best practices, but nevertheless, this isn't a thing that I would keep blocking the PR.
| oc login | ||
| <span class="nowrap">--token={{.AccessToken}}</span> | ||
| <span class="nowrap">--server={{.PublicMasterURL}}</span> | ||
| <button onclick="navigator.clipboard.writeText(commands.login)">Copy</button> |
There was a problem hiding this comment.
Shouldn't we add some feedback for the user, like changing the button text to "Copied!" or so?
There was a problem hiding this comment.
This would add more complexity, which is something I wanted to avoid.
The page template is missing standard HTML structure, including <!doctype html>, <head>, <body> etc. which cause browsers to render this page it in "quirks" mode.
I could try to add some custom-styled "Copied!" feedback text upon clicking the button, but do we really need it?
| oc login | ||
| <span class="nowrap">--token={{.AccessToken}}</span> | ||
| <span class="nowrap">--server={{.PublicMasterURL}}</span> | ||
| <button onclick="navigator.clipboard.writeText(commands.login)">Copy</button> |
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText
Isn't it best practice to handle the error somehow?
There was a problem hiding this comment.
We just call the clipboard.writeText function without handling the resulting Promise.
This function is async and resolves when the clipboard's contents have been updated. If we had a "Copied!" feedback text implemented, we'd be handing the Promise, but currently we don't need that.
I'm not sure we need an explicit error handler here, unless we want to implement the feedback text, could be something like "Failed to copy the snippet to clipboard, see browser logs for details" or similar.
In practice, clipboard.writeText function could fail if the page was forbidden to copy text to clipboard via setting page permissions, e.g. for Chromium based browsers [1]:
Writing requires either the
clipboard-writepermission or transient activation.
[1] https://developer.mozilla.org/en-US/docs/Web/API/Clipboard_API#security_considerations
There was a problem hiding this comment.
@vojtechszocs Maybe just throw up an alert on an error? Something like...? "Copy to clipboard failed. Select the content to copy it manually."
In practice, I'd expect clipboard write to almost never fail.
vojtechszocs
force-pushed
the
token-page-copy-to-clipboard
branch
from
c9a96d7 to
412eb21
Compare
|
I wish there would have been a more detailed explanation why it is fine to get rid of the intermediate CSRF step, but after bouncing my head for an hour on this topic, the worst thing I could imagine is that a disgruntled Alice could sneak in her tokens on your page. |
|
I will approve once @everettraven lgtm'ed it... or @liouk |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
I'm concerned about us making a breaking change to the acceptable requests here - because it is out there, and has been out there for a while, I would not be surprised if there is some kind of end user workflow that depends on sending a properly formed POST request to display the token.
Additionally, do we have any kind of testing in place for UX/UI related changes we can update / put in place here?
If there is not existing testing infrastructure here it isn't the end of the world, but if it exists we should make sure the UX changes are appropriately tested.
| <input type="hidden" name="code" value="{{.Code}}"> | ||
| <input type="hidden" name="csrf" value="{{.CSRF}}"> | ||
| <button type="submit"> | ||
| Display Token |
There was a problem hiding this comment.
We can't remove this. This was there to prevent cross-site request forgery.
/hold
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughToken HTML rendering moved client-side: server now injects JS-safe strings into the template; three empty Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Browser
participant Clipboard
participant User
Server->>Browser: Serve template with `AccessTokenJSStr` and `PublicMasterURLJSStr`
Browser->>Browser: Populate three `<pre>` placeholders via `codeSnippet` (render spans, add Copy/Show buttons)
User->>Browser: Click "Copy"
Browser->>Clipboard: navigator.clipboard.writeText(commandText)
Clipboard-->>Browser: Resolve/Reject
Browser-->>User: Update button state (Copied / Error / Show toggles)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
vojtechszocs
force-pushed
the
token-page-copy-to-clipboard
branch
from
412eb21 to
34ca6e5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/server/tokenrequest/tokenrequest.go (1)
147-148:⚠️ Potential issue | 🟠 MajorAvoid
template.JSStrfor runtime values and avoidinnerHTMLrendering for snippets.On Line 147-Line 148 and Line 183-Line 186, casting runtime data to
template.JSStrmarks it as trusted. Combined with Line 274-Line 275 (innerHTML), this weakens escaping guarantees and can become an injection vector if token/URL formats ever change.Proposed fix
@@ - data.AccessTokenJSStr = template.JSStr(data.AccessToken) - data.PublicMasterURLJSStr = template.JSStr(data.PublicMasterURL) + data.AccessTokenJSStr = data.AccessToken + data.PublicMasterURLJSStr = data.PublicMasterURL @@ - AccessToken string - AccessTokenJSStr template.JSStr - PublicMasterURL string - PublicMasterURLJSStr template.JSStr - LogoutURL string + AccessToken string + AccessTokenJSStr string + PublicMasterURL string + PublicMasterURLJSStr string + LogoutURL string @@ - const snippetHTML = textBlocks - .map((text) => '<span class="nowrap">' + text + '</span>') - .join(' '); + const renderSnippet = (container) => { + container.textContent = ''; + textBlocks.forEach((text, idx) => { + if (idx > 0) container.appendChild(document.createTextNode(' ')); + const span = document.createElement('span'); + span.className = 'nowrap'; + span.textContent = text; + container.appendChild(span); + }); + }; @@ - showButton.onclick = () => { - e.innerHTML = snippetHTML; - e.appendChild(copyButton); - }; + showButton.onclick = () => { + renderSnippet(e); + e.appendChild(copyButton); + };In Go's html/template, does casting user/runtime strings to template.JSStr bypass contextual escaping, and is this recommended for untrusted input in <script> string contexts?Also applies to: 183-186, 243-245, 274-275, 295-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/tokenrequest/tokenrequest.go` around lines 147 - 148, The code currently casts runtime values to template.JSStr (data.AccessTokenJSStr, data.PublicMasterURLJSStr) and later relies on innerHTML, which bypasses contextual escaping; remove those template.JSStr casts and instead pass the raw strings (data.AccessToken, data.PublicMasterURL) into the template, update the template to avoid innerHTML by setting textContent or using data-attributes (or render a JSON-encoded value via json.Marshal and only mark the already-encoded JSON safe with template.JS), and ensure any place that previously read AccessTokenJSStr/PublicMasterURLJSStr in the template is updated to use the safe rendering approach so runtime values are escaped by html/template rather than trusted unconditionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/server/tokenrequest/tokenrequest.go`:
- Around line 147-148: The code currently casts runtime values to template.JSStr
(data.AccessTokenJSStr, data.PublicMasterURLJSStr) and later relies on
innerHTML, which bypasses contextual escaping; remove those template.JSStr casts
and instead pass the raw strings (data.AccessToken, data.PublicMasterURL) into
the template, update the template to avoid innerHTML by setting textContent or
using data-attributes (or render a JSON-encoded value via json.Marshal and only
mark the already-encoded JSON safe with template.JS), and ensure any place that
previously read AccessTokenJSStr/PublicMasterURLJSStr in the template is updated
to use the safe rendering approach so runtime values are escaped by
html/template rather than trusted unconditionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c316f08e-9d51-4f66-9600-9d42e3910e7e
📒 Files selected for processing (1)
pkg/server/tokenrequest/tokenrequest.go
|
Sorry, this PR got stuck in a limbo state.
AFAIK we have customer RFEs that suggested removing the intermediate "Display Token" form submit (CSRF) step since it's one more page to go through when accessing token data. That said, I've removed this change from the PR to preserve the existing behavior.
That makes perfect sense 👍 |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Changes seem fine to me. Will defer to @spadgett for any web-specific concerns.
|
/lgtm |
|
PR updated to ensure that snippet content is treated as text only. This can be tested by manually changing the snippet text blocks inside codeSnippet(document.getElementById('token'), [
'{{.AccessTokenJSStr}} <foo>&test</foo>',
]); |
|
/approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven, spadgett, vojtechszocs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
|
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
This PR modifies the token display page by adding "Show" and "Copy" buttons.
Initially, all snippets have their content hidden using
***placeholder text.Click on "Show" button to reveal the content and make the "Show" button disappear for that snippet.
Click on "Copy" button to copy the content to clipboard using the standard Clipboard API.
Clicking the "Copy" button changes its text and disables it for 3 seconds, then resets the button to its normal state.
console.errorSummary by CodeRabbit