(spyglass/lenses) allow configuration sandbox permissions #296

norrs · 2024-10-10T13:42:30Z

This provides the ability to configure iframe sandbox permissions pr lense. This allows the operator of the prow installation to define which permissions it trust to each lense.

PR comes from the ideas and discussions in
#294

This provides the ability to configure iframe sandbox permissions pr lense. This allows the operator of the prow installation to define which permissions it trust to each lense. PR comes from the ideas and discussions in kubernetes-sigs#294 Signed-off-by: Roy Sindre Norangshol <[email protected]>

k8s-ci-robot · 2024-10-10T13:42:40Z

Hi @norrs. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

netlify · 2024-10-10T13:42:48Z

✅ Deploy Preview for k8s-prow ready!

Name	Link
🔨 Latest commit	`44d0098`
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-prow/deploys/6793b7caaddc96000822fab9
😎 Deploy Preview	https://deploy-preview-296--k8s-prow.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

norrs · 2024-10-10T13:42:57Z

cmd/checkconfig/testdata/combined.yaml

+          iframe_sandbox_permissions:
+          - allow-scripts
+          - allow-popups
+          - allow-popups-to-escape-sandbox


Updated this as it looked like a sane idea to do if this is supposed to demostrate a full configuration file example.

norrs · 2024-10-10T13:45:16Z

pkg/spyglass/lenses/buildlog/lens.go

+var defaultSandboxPermissions = strings.Join(
+	[]string{
+		"allow-scripts",
+		"allow-top-navigation",
+		"allow-popups",
+		"allow-same-origin",
+	},
+	" ",
+)


This is defaultSandboxPermissions for the parsedConfig, tho I personally like to work with arrays instead of strings with foo bar zar.

Not sure if you prefer this or the allow-scripts allow-top-navigation allow-popups allow-same-origin instead.

Please let me know, I'll update if you want to change it.

Suggestion is to do another PR after this to adjust the defaults to not combine allow-same-origin together with allow-scripts, but I suppose this needs to be communicated to the users of the prow installation, so they are aware of the ability to configure this pr lense.

Arrays sound reasonable to me, and I think it's easier to catch differences.

And that sounds good to me.

michelle192837 · 2024-10-29T23:26:05Z

/ok-to-test

norrs · 2024-11-18T11:29:43Z

Do you need anything more from me on this?

norrs · 2025-01-09T18:00:07Z

Closed #294 in favor of this PR.
Please note that the referred PR has the background for why this PR was created in the first place.

Doing a ping/cc here from the people who were active in 294 ( @michelle192837 @smg247 @BenTheElder . )

Would be nice if we could get this in? 🙏

michelle192837 · 2025-01-09T18:46:38Z

Thanks for the ping and apologies for the delay, this dropped off my radar ><

I'll review today!

michelle192837

LGTM aside from a minor nit.

michelle192837 · 2024-10-29T23:23:26Z

pkg/spyglass/lenses/buildlog/lens.go

+var defaultSandboxPermissions = strings.Join(
+	[]string{
+		"allow-scripts",
+		"allow-top-navigation",
+		"allow-popups",
+		"allow-same-origin",
+	},
+	" ",
+)


Arrays sound reasonable to me, and I think it's easier to catch differences.

And that sounds good to me.

michelle192837 · 2025-01-09T19:29:46Z

pkg/spyglass/lenses/buildlog/lens_test.go

@@ -61,6 +62,14 @@ func TestGetConfig(t *testing.T) {
 				}
 				return d
 			}(),
+		}, {


nit: Add test case verifying what the iframe permissions are if none are specified.

Added test @michelle192837 , good nit 👍

if providing sandbox iframe sandbox permissions configuration which is empty, it should provide no prvileges for the sandbox and not fall back to the defaults. if you want defaults, you should not configure the iframe.sandbox_permissions. Signed-off-by: Roy Sindre Norangshol <[email protected]>

michelle192837

Neat, thanks again for the changes!

norrs · 2025-01-31T23:09:29Z

Seems like we need an approval from cmd/checkconfig/OWNERS: @chases2 , @stevekuznetsov or @cjwagner

michelle192837 · 2025-01-31T23:17:31Z

Those folks are no longer working on Prow unfortunately. I think this should get approval from someone in https://github.com/kubernetes-sigs/prow/blob/main/OWNERS until that OWNERS file is fixed.

(Pulling randomly) @petr-muller or @matthyx ?

matthyx

/lgtm

k8s-ci-robot · 2025-02-01T07:31:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matthyx, michelle192837, norrs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/checkconfig/OWNERS~~ [matthyx]
~~cmd/deck/OWNERS~~ [matthyx,michelle192837]
~~pkg/spyglass/OWNERS~~ [matthyx,michelle192837]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from michelle192837 and smg247 October 10, 2024 13:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 10, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 10, 2024

norrs commented Oct 10, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 29, 2024

ghost mentioned this pull request Nov 2, 2024

1.tears od #313

Closed

norrs mentioned this pull request Jan 9, 2025

(spyglass/lenses/html) add allow-forms to sandbox #294

Closed

michelle192837 reviewed Jan 9, 2025

View reviewed changes

norrs force-pushed the norrs/spyglass/allow-configuration-of-sandbox-permissions-per-lense branch from fe2591e to 1cd2aad Compare January 24, 2025 15:40

norrs force-pushed the norrs/spyglass/allow-configuration-of-sandbox-permissions-per-lense branch from 1cd2aad to 44d0098 Compare January 24, 2025 15:54

norrs requested a review from michelle192837 January 24, 2025 16:11

michelle192837 approved these changes Jan 30, 2025

View reviewed changes

k8s-ci-robot assigned michelle192837 Jan 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2025

matthyx approved these changes Feb 1, 2025

View reviewed changes

k8s-ci-robot assigned matthyx Feb 1, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2025

k8s-ci-robot merged commit c3a90fc into kubernetes-sigs:main Feb 1, 2025
11 checks passed

This was referenced Feb 3, 2025

PR (spyglass/lenses) allow configuration sandbox permissions #296 broke spyglass #369

Open

Revert "Bumping Prow and Prow - test-infra and Prow - ci-infra (#3134)" gardener/ci-infra#3142

Merged

upodroid mentioned this pull request Feb 4, 2025

Update boskos to v20250131-478cd5d, prow to v20250205-e871edfd1 and ghproxy as needed kubernetes/k8s.io#7712

Open

petr-muller mentioned this pull request Feb 4, 2025

Revert "(spyglass/lenses) allow configuration sandbox permissions" #371

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(spyglass/lenses) allow configuration sandbox permissions #296

(spyglass/lenses) allow configuration sandbox permissions #296

norrs commented Oct 10, 2024

k8s-ci-robot commented Oct 10, 2024

netlify bot commented Oct 10, 2024 •

edited

Loading

norrs Oct 10, 2024

norrs Oct 10, 2024

norrs Oct 10, 2024

michelle192837 Oct 29, 2024

michelle192837 commented Oct 29, 2024

norrs commented Nov 18, 2024

norrs commented Jan 9, 2025

michelle192837 commented Jan 9, 2025

michelle192837 left a comment

michelle192837 Oct 29, 2024

michelle192837 Jan 9, 2025

norrs Jan 24, 2025

michelle192837 left a comment

norrs commented Jan 31, 2025

michelle192837 commented Jan 31, 2025

matthyx left a comment

k8s-ci-robot commented Feb 1, 2025

(spyglass/lenses) allow configuration sandbox permissions #296

(spyglass/lenses) allow configuration sandbox permissions #296

Conversation

norrs commented Oct 10, 2024

k8s-ci-robot commented Oct 10, 2024

netlify bot commented Oct 10, 2024 • edited Loading

✅ Deploy Preview for k8s-prow ready!

norrs Oct 10, 2024

Choose a reason for hiding this comment

norrs Oct 10, 2024

Choose a reason for hiding this comment

norrs Oct 10, 2024

Choose a reason for hiding this comment

michelle192837 Oct 29, 2024

Choose a reason for hiding this comment

michelle192837 commented Oct 29, 2024

norrs commented Nov 18, 2024

norrs commented Jan 9, 2025

michelle192837 commented Jan 9, 2025

michelle192837 left a comment

Choose a reason for hiding this comment

michelle192837 Oct 29, 2024

Choose a reason for hiding this comment

michelle192837 Jan 9, 2025

Choose a reason for hiding this comment

norrs Jan 24, 2025

Choose a reason for hiding this comment

michelle192837 left a comment

Choose a reason for hiding this comment

norrs commented Jan 31, 2025

michelle192837 commented Jan 31, 2025

matthyx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 1, 2025

netlify bot commented Oct 10, 2024 •

edited

Loading