fix: DebugBar block by CSP #8411

YapsBridging · 2024-01-12T03:24:12Z

Description
Fixes #8405

Remove inline style : javascrip:void ,onclick,display none
Update CSS for show tab
Add switchClass func to switch class CSS
Add eventListener

BEFORE

AFTER

Steps to Reproduce
set CSPEnabled = true,
set ENVIRONMENT to development

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

michalsn

Look nice, but please fix the styling of the CSS and JS files.

admin/css/debug-toolbar/toolbar.scss

system/Debug/Toolbar/Views/toolbar.js

YapsBridging · 2024-01-13T09:48:00Z

Thank you @michalsn for the explanation,
previously I was confused about what was wrong,...I'll try it

kenjis · 2024-01-22T04:55:03Z

We don't use merge commits in PR branch.
If you can use git, please use git rebase.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

kenjis · 2024-01-22T04:58:20Z

app/Views/welcome_message.php

@@ -309,7 +331,8 @@

 <!-- SCRIPTS -->

-<script>
+<script {csp-script-nonce}>


Suggested change

<script {csp-script-nonce}>

<script <?= csp_script_nonce() ?>>

thanks @kenjis , but why dont use {csp-script-nonce} ?
like line 12 welcome_message.php, use {csp-style-nonce}

Okay, you are correct. The line 12 uses <style {csp-style-nonce}>.
So using it here is okay.

I personally think we don't need to use {csp-style-nonce} if we can use the csp_script_nonce() function. It is a CI4 global function and we can always use it in PHP scripts.

i think so too..
if csp is enable ,it must following change the placeholder tagNonce , cause If not change and if an attacker injects a string like <script {csp-script-nonce}> it might become the real nonce attribute with functionality.
https://www.codeigniter.com/user_guide/outgoing/csp.html

so the best we can use csp_script_nonce()

kenjis · 2024-01-22T05:01:48Z

Please sign all commits.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

YapsBridging · 2024-01-22T12:56:58Z

We don't use merge commits in PR branch. If you can use git, please use git rebase. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

thanks @kenjis , This is my first time making a PR,
sorry if there are any mistakes,
so what can I do to fix this with a rebase? can you tell me the steps?

I have read the link above, but I don't understand how to fix it, can you help me

and sorry my bad english.

kenjis · 2024-01-23T01:09:17Z

@YapsBridging Welcome to CodeIgniter4!

First, I recommend you set up your git to sign all commits (in this repository).

To configure your Git client to sign commits by default for a local repository, in Git versions 2.0.0 and above, run git config commit.gpgsign true. To sign all commits by default in any local repository on your computer, run git config --global commit.gpgsign true.
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

See also https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/signing.md

kenjis · 2024-01-23T01:16:38Z

Set upstream repository:

git remote add upstream git@github.com:codeigniter4/CodeIgniter4.git

Fetch the latest upstream:

git fetch upstream

Rebase your PR branch:

git switch csp
git rebase upstream/develop

Force push:

git push --force-with-lease origin HEAD

YapsBridging · 2024-01-24T13:46:23Z

Set upstream repository:
git remote add upstream git@github.com:codeigniter4/CodeIgniter4.git
Fetch the latest upstream:
git fetch upstream
Rebase your PR branch:
git switch csp
git rebase upstream/develop
Force push:
git push --force-with-lease origin HEAD

thank @kenjis
I have tried the steps above, and I got a big problem..haha, sorry im a newbe..

so can i create a new PR with new Branch and close/delete this one?

kenjis · 2024-01-24T21:57:49Z

Oh, you don't use git. Creating a new PR is no problem!

But if you could use git, you can change your commits easily without creating new PRs.
If you want to learn git, I would help you.
Or if you help to improve our contribution guide, I would appreciate it.

YapsBridging · 2024-01-25T12:14:08Z

Oh, you don't use git. Creating a new PR is no problem!

But if you could use git, you can change your commits easily without creating new PRs. If you want to learn git, I would help you. Or if you help to improve our contribution guide, I would appreciate it.

https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#how-to-guide

https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#syncing-your-repository

thanks @kenjis , of course i want,

i'm glad ,if u want to help me to,

basically i use git with command prompt but just standard command pull, push ,commit create branch,switch, check log,etc,,

one of the problems is when i try to signing to all old commit with

git rebase -i --root --exec 'git commit --amend --no-edit --no-verify -S'

https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

its also overwrites other people's commits with my signature
i dont understand how this work..LOL

maybe because of the merge.

LoL..
so can I go back to the old commit before I merged, before all these problems happened?

kenjis · 2024-01-25T12:59:04Z

You did not push your local branch to the remote branch in GitHub.
So you still have the remote branch (copy) in your local repository.

$ git switch develop
$ git branch -D csp # delete local csp branch
$ git checkout csp
branch 'csp' set up to track 'origin/csp'.
Switched to a new branch 'csp'

kenjis · 2024-01-25T13:02:58Z

Or you can clone your repository in GitHub again.

git clone git@github.com:YapsBridging/CodeIgniter4.git
git checkout csp

kenjis · 2024-01-25T13:13:42Z

I recommend:

As a faster alternative, you can still securely sign commits without the -S option in git commit by setting git config --global commit.gpgsign true and git config --global user.signingkey 3AC5C34371567BD2 to all local repositories. Without the --global option, the change is applied to one local repository only.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

and if you set it, git rebase also automatically signs all rebased commits.

If you follow the instruction, the commit log would be following:

a7a441716c (HEAD -> csp) update style & js
795de34a07 update style & js
539c8b3c02 fix changes SCSS files did not match the expected CSS output
915b3251f1 remove inline style debugbar,welcome_message for CSP
7c426fa1dd update css for show tab debugbar
219011070a (upstream/develop) Merge pull request #8453 from kenjis/test-postgre-upsertBatch

YapsBridging · 2024-01-25T13:44:35Z

@kenjis , Ok I'll try it

YapsBridging · 2024-01-25T14:58:48Z

greate !..

one step again
i try to Force push:
git push --force-with-lease origin HEAD ,
and i get this message..what happen?

kenjis · 2024-01-25T23:49:18Z

You did git rebase successfully!

The error on push seems an issue about your token permission.
See https://stackoverflow.com/questions/64059610/how-to-resolve-refusing-to-allow-an-oauth-app-to-create-or-update-workflow-on

I use SSH auth, so I have never seen that error.

$ git remote -v
origin	git@github.com:YapsBridging/CodeIgniter4.git (fetch)
origin	git@github.com:YapsBridging/CodeIgniter4.git (push)
upstream	git@github.com:codeigniter4/CodeIgniter4.git (fetch)
upstream	git@github.com:codeigniter4/CodeIgniter4.git (push)

YapsBridging · 2024-02-02T03:48:33Z

hi @kenjis , I have completed the permission token permission,
i try again
git push --force-with-lease origin HEAD ,

now i get message

can u tell me how to fix it ?

kenjis · 2024-02-02T07:04:37Z

It shows someone (probably you?) added commit(s) that is not included in your local branch to the remote branch on GitHub.

If you are sure the local branch is no problem, try:

# Update your local branch, because the upstream develop advanced.
git fetch upstream
git switch csp
git rebase upstream/develop
# Force push with no check.
git push -f origin HEAD

kenjis · 2024-02-10T01:08:13Z

@YapsBridging Can't you push to your PR branch?

YapsBridging · 2024-02-10T01:20:49Z

@YapsBridging Can't you push to your PR branch?

@kenjis , I thought I had completed the token permission. but apparently not, I need some more time to push again..., maybe with a new branch, I'm also afraid of lots of conflicts when I push using the same branch

kenjis · 2024-02-10T01:25:46Z

The steps in #8411 (comment) did not work?

Or pushing as a new branch and creating a new PR would be fine.

YapsBridging · 2024-02-10T02:07:57Z

The steps in #8411 (comment) did not work?

Or pushing as a new branch and creating a new PR would be fine.

I haven't tried it yet,
I'm afraid that when I push there will be lots of conflicts, I have already committed to the remote branch, and also the merge that I did is already on the remote branch. CSP's remote branch contains merge and commit, and my local branch doesn't

kenjis · 2024-02-10T02:28:45Z

I don't understand the conflicts you are afraid of.
Force push pushes your local branch as it is. It can remove the commits in the remote branch on GitHub.

YapsBridging · 2024-02-10T03:47:33Z

I don't understand the conflicts you are afraid of. Force push pushes your local branch as it is. It can remove the commits in the remote branch on GitHub.

ow, i dont know about that,,,thanks @kenjis , i will try push again

paulbalandan · 2024-02-10T15:06:09Z

admin/css/debug-toolbar/toolbar.scss

+#ci-database.debug-bar-dblock,
+#ci-files.debug-bar-dblock,
+#ci-routes.debug-bar-dblock,
+#ci-events.debug-bar-dblock,
+#ci-history.debug-bar-dblock,
+#ci-vars.debug-bar-dblock,
+#ci-config.debug-bar-dblock,
+#ci-timeline.debug-bar-dblock {


I believe this can be simplified to one CSS class, .debug-bar-dblock, instead of individually showing all uses in #ci-* ids. If there would be a new id, say #ci-dump, it would need to be added here again.

You're right..thanks @paulbalandan

kenjis · 2024-02-10T21:40:07Z

app/Views/welcome_message.php

@@ -150,6 +150,26 @@
 .further h2:first-of-type {
 padding-top: 0;
 }
+ .f1 {


What is f?

@kenjis its style for font

It seems we don't use names like f1.
Can you change to meaningful names?
font-something?

ok @kenjis i'll change it

kenjis · 2024-02-12T05:56:26Z

macOS and Firefox.

Before:

After:

michalsn

LGTM!

kenjis

Thank you!

crustamet · 2024-02-12T11:53:57Z

This is great, but there is one little issue if only one case exist... while having

for example
<base href="localhost:8080">
in your main html the buttons on debugbar always redirect to base if you click the buttons "Database", "Views", "Logs", "Files"

I always get redirected to "homepage" using this new debugbar. i think this happened before but without the CSP Fix while clicking on them nothing happened...

The fix is as following remove the href="#" from the button links

	<a href="#" data-toggle="datatable" data-table="<?= strtolower(str_replace(' ', '-', $heading)) ?>">
		<h2><?= $heading ?></h2>
	</a>

	TO

	<a data-toggle="datatable" data-table="<?= strtolower(str_replace(' ', '-', $heading)) ?>">
		<h2><?= $heading ?></h2>
	</a>

kenjis · 2024-02-12T23:46:30Z

What do you mean?

It works with no problem with the following settings.

diff --git a/app/Config/App.php b/app/Config/App.php
index 21b4df2052..aa433bf847 100644
--- a/app/Config/App.php
+++ b/app/Config/App.php
@@ -174,5 +174,5 @@ class App extends BaseConfig
      * @see http://www.html5rocks.com/en/tutorials/security/content-security-policy/
      * @see http://www.w3.org/TR/CSP/
      */
-    public bool $CSPEnabled = false;
+    public bool $CSPEnabled = true;
 }
diff --git a/app/Views/welcome_message.php b/app/Views/welcome_message.php
index 86a6e39de3..88001613ca 100644
--- a/app/Views/welcome_message.php
+++ b/app/Views/welcome_message.php
@@ -2,6 +2,7 @@
 <html lang="en">
 <head>
     <meta charset="UTF-8">
+    <base href="localhost:8080">
     <title>Welcome to CodeIgniter 4!</title>
     <meta name="description" content="The small framework with powerful features">
     <meta name="viewport" content="width=device-width, initial-scale=1.0">

YapsBridging · 2024-02-13T08:39:42Z

@crustamet i see, cause tag base add value to All relative link in the page
@kenjis this will happen if the url page different from the tag base

ex in
head tag base http://localhost:8080
but page url http://localhost:8080/dashboard

so the tag/button debugbar will be http://localhost:8080/#

before i change javascript:void(0) to # , because javascript:void(0) was blocked by CSP

YapsBridging · 2024-02-13T14:20:08Z

but in my opinion, if you use tag base, maybe other plugins will also be affected. If you use other plugins, you don't know whether the plugin you are using contains relative links.

the tag Base will add value to All relative link in the page

So in my opinion using the base tag is not appropriate, it can cause unexpected problems.

that's just my opinion

crustamet · 2024-02-13T14:27:04Z

but in my opinion, if you use tag base, maybe other plugins will also be affected. If you use other plugins, you don't know whether the plugin you are using contains relative links.
the tag Base will add value to All relative link in the page
So in my opinion using the base tag is not appropriate, it can cause unexpected problems.
that's just my opinion

It only causes problems if you have already create the project that is not using this base option and then you use it when the project is finished :)) it doesnt work like that, you use it from the beginning of the project creation so it helps you organize everything better

I don't think this matters for is appropriate or not, it does not break any functionalities for me, and its better because i load all my links with this base link.

And i don't think it matters in this situation either, because what ? i added a valid base meta tag and the debugbar is not working ? :))) the debugbar should work whatever you do with your code.

YapsBridging · 2024-02-13T14:47:38Z

ok @crustamet, you're right, maybe this is the reason why the debugbar button uses javascript:void(0) not #

crustamet · 2024-02-13T14:54:15Z

why in the world would you want to have a hashtag in your links without wanting them there ?
the hashtag was before used in the old times to remember what you opened..

Here in the new debugbar it does that by adding data-(*attrs) to the links so now there is no need for href="#" at this point
It should be removed completely from the links

It would be better to have them as buttons instead of links but yeah there is bug with <base> tag the debugbar not working..
It only works on the homepage of the project

YapsBridging · 2024-02-13T15:48:34Z

ow..i dont know about history of the hastag.. i know the single hastag, href="#"
one of the ways to create anchors that don't link to anything. so i use that for alternative change javascript:void..
however you're right we can remove href to create anchors that don't link to anything

crustamet · 2024-02-13T16:13:00Z

you should really make them with buttons 😆 but a tags without href so weird because now you will loose pointer cursor xD

YapsBridging · 2024-02-13T16:32:04Z

ow i forgot...Thank you @kenjis , for teaching Git....:))

kenjis · 2024-02-13T21:31:29Z

@kenjis this will happen if the url page different from the tag base

ex in
head tag base http://localhost:8080
but page url http://localhost:8080/dashboard

so the tag/button debugbar will be http://localhost:8080/#

Cannot reproduce with <base href="localhost:8080"> on Firefox.
The tag/button debugbar is http://localhost:8080/dashboard#

kenjis · 2024-02-13T21:35:59Z

Oh, I was able to reproduce with <base href="http://localhost:8080"> and http://localhost:8080/dashboard.

YapsBridging changed the title ~~Fix: [DebugBar] block by CSP #8405~~ fix: DebugBar block by CSP Jan 12, 2024

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 12, 2024

michalsn requested changes Jan 13, 2024

View reviewed changes

kenjis reviewed Jan 22, 2024

View reviewed changes

kenjis mentioned this pull request Feb 8, 2024

Bug: [DebugBar] block by CSP #8405

Closed

YapsBridging added 2 commits February 10, 2024 13:38

update css for show tab debugbar

51baeff

remove inline style debugbar,welcome_message for CSP

e48a4f3

YapsBridging added 3 commits February 10, 2024 13:38

fix changes SCSS files did not match the expected CSS output

1a53368

update style & js

1f02846

update style & js

0732719

YapsBridging force-pushed the csp branch from 406ae36 to 0732719 Compare February 10, 2024 06:04

kenjis changed the title ~~fix: DebugBar block by CSP~~ fix: DebugBar block by CSP Feb 10, 2024

kenjis requested a review from michalsn February 10, 2024 08:55

paulbalandan reviewed Feb 10, 2024

View reviewed changes

kenjis reviewed Feb 10, 2024

View reviewed changes

update css toolbar,welcome_message

c563325

michalsn approved these changes Feb 12, 2024

View reviewed changes

kenjis requested a review from paulbalandan February 12, 2024 08:27

paulbalandan approved these changes Feb 12, 2024

View reviewed changes

kenjis approved these changes Feb 12, 2024

View reviewed changes

kenjis merged commit 3b1d8e3 into codeigniter4:develop Feb 12, 2024
63 checks passed

ddevsr mentioned this pull request Mar 1, 2024

Bug: Debugbar Click(toggle) Scrolls Document to Top #8594

Closed

	<script {csp-script-nonce}>
	<script <?= csp_script_nonce() ?>>

fix: DebugBar block by CSP #8411

fix: DebugBar block by CSP #8411

Conversation

YapsBridging commented Jan 12, 2024 • edited Loading

michalsn left a comment

Choose a reason for hiding this comment

YapsBridging commented Jan 13, 2024

kenjis commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented Jan 22, 2024

YapsBridging commented Jan 22, 2024

kenjis commented Jan 23, 2024

kenjis commented Jan 23, 2024

YapsBridging commented Jan 24, 2024

kenjis commented Jan 24, 2024

YapsBridging commented Jan 25, 2024

kenjis commented Jan 25, 2024

kenjis commented Jan 25, 2024

kenjis commented Jan 25, 2024

YapsBridging commented Jan 25, 2024

YapsBridging commented Jan 25, 2024

kenjis commented Jan 25, 2024

YapsBridging commented Feb 2, 2024

kenjis commented Feb 2, 2024

kenjis commented Feb 10, 2024

YapsBridging commented Feb 10, 2024

kenjis commented Feb 10, 2024

YapsBridging commented Feb 10, 2024

kenjis commented Feb 10, 2024

YapsBridging commented Feb 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YapsBridging Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis commented Feb 12, 2024

michalsn left a comment

Choose a reason for hiding this comment

kenjis left a comment

Choose a reason for hiding this comment

crustamet commented Feb 12, 2024 • edited by kenjis Loading

kenjis commented Feb 12, 2024

YapsBridging commented Feb 13, 2024

YapsBridging commented Feb 13, 2024

crustamet commented Feb 13, 2024

YapsBridging commented Feb 13, 2024

crustamet commented Feb 13, 2024

YapsBridging commented Feb 13, 2024

crustamet commented Feb 13, 2024 • edited Loading

YapsBridging commented Feb 13, 2024

kenjis commented Feb 13, 2024

kenjis commented Feb 13, 2024

YapsBridging commented Jan 12, 2024 •

edited

Loading

YapsBridging Feb 10, 2024 •

edited

Loading

crustamet commented Feb 12, 2024 •

edited by kenjis

Loading

crustamet commented Feb 13, 2024 •

edited

Loading