#565 Provide optional SRI hash when displaying embed code #730

raLaaaa · 2022-07-28T08:04:53Z

Hey, first draft for the implementation of an optional SRI hash for the script tag.

The last required step is not working yet (by that I mean pulling the embedding script into the auditorium build file stage to generate the hash). Hopefully I can finalize everything on the weekend and already add your feedback / improvements.

@hendr-ik if you want I'll also update the UI towards a using a simple checkbox.

… hash to docker-compose. Added UI&Integrity hash to audtiroum

m90 · 2022-07-28T08:07:51Z

auditorium/gulpfile.js

@@ -13,6 +13,7 @@ var buffer = require('vinyl-buffer')
 var gap = require('gulp-append-prepend')
 var Readable = require('stream').Readable
 var linguasFile = require('linguas-file')
+var CryptoJS = require('crypto-js')


As this is now exclusively running on the server, can we use the stdlib crypto module and not add a dependency?

m90 · 2022-07-28T10:01:38Z

auditorium/src/views/components/auditorium/embed-code.js

+  const snippetWithSRI = `<script async src="${window.location.origin}/script.js" data-account-id="${model.account.accountId}" integrity="sha256-${process.env.SCRIPT_INTEGRITY_HASH}"></script>`
+  const [useSnippetWithSRI, setActive] = useState(false);
+  let embeddedSnipped = useSnippetWithSRI ? snippetWithSRI : snippet;
+  let buttonText = useSnippetWithSRI ? 'Hide integrity hash' : 'Show with integrity hash';


As Offen is translated into multiple languages, all copy needs to be localized properly. Doing this is relatively easy: a global __ function is available in all modules that you can wrap a string in, marking as "to be localized":

__('Hide integrity hash')

m90 · 2022-07-28T10:03:05Z

auditorium/src/views/components/auditorium/embed-code.js

@@ -59,17 +70,22 @@ const EmbedCode = (props) => {
      <Paragraph class='ma0 mb3'>
        {__('In case you are serving multiple domains from your Offen instance, please double check that the domain in this snippet matches the target account.')}
      </Paragraph>
+      <div class="flex items-end">
+        <div class="w-100 tr bb b--light-gray">
+          <button onClick={toggleScriptDisplay} class='pointer w-100 w-auto-ns fw1 f7 tc bn dib br1 ph2 pv1 black bg-black-10'><span class={classnames('ml2', 'dib', 'label-toggle', 'label-toggle', !useSnippetWithSRI? null : 'label-toggle--rotate')}></span> {buttonText}</button>


With classnames you don't need to add ternary statements yourself, instead you can have the library conditionally apply classnames:

classnames('a', 'b' {'label-toggle--rotate': useSnippetWithSRI })

lgtm-com · 2022-07-29T07:51:06Z

This pull request introduces 1 alert when merging ac23ced into c7aed87 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

raLaaaa · 2022-07-29T07:55:50Z

@m90 thank you for your feedback.
I hopefully tackled most of the stuff in the latest commits.

Most importantly I changed towards the crypto library from node.
How ever copying the index.js from the script folder only worked for when running the script stage before the auditorium stage. Otherwise the file can not be found. ac23ced

Docker will figure out that the auditorium stage now depends on the script stage and wait for it to build.

Is there a special trick to invoke this behavior you mentioned?

Also: currently the generated sha-256 hash always differs from the browser enforced integrity hash.
I assume that is because the browser generates the hash from a minified version (?) whereas I'm currently calculating the hash from the clear text script file. I'm working on that.

Furthermore my formatter seems to differ a bit from yours. Which one do you use?

m90 · 2022-07-29T15:22:49Z

Furthermore my formatter seems to differ a bit from yours. Which one do you use?

The entire repo uses standard at version 14

offen/auditorium/package.json

Line 67 in c7aed87

"standard": "^14.3.1",

- it would be much appreciated if you could use this so the diff noise is as low as possible. Right now, it's a bit hard to tell which changes are formatting, and which ones are feature related.

You can check whether standard likes your changes by running the tests locally:

docker-compose run auditorium npm t

Is there a special trick to invoke this behavior you mentioned?

Not really a trick. If you look at the compiler stage of the build Dockerfile, you can see how it copies in the artifacts generated by the script stage here:

offen/build/Dockerfile.build

Line 111 in c7aed87

    
           COPY --from=script /code/script/dist /go/src/github.com/offen/offen/server/public/static

You can now do the same in the auditorium stage, copying these files to the location expected by the Gulpfile.

m90 · 2022-07-29T15:24:01Z

build/Dockerfile.build

 WORKDIR /code/deps
 RUN npm ci
-COPY ./script /code/script
+COPY ./auditorium /code/auditorium
+COPY --from=script ./code/script/index.js /code/auditorium/script/index.js


This copies the entrypoint file when instead we need to copy the bundled versions. See my comment on how to do this.

Shouldn't this invoke the behavior we need?

COPY --from=script /code/script/dist/index.js /script/script.js

There is a bundle created per locale (the script logs user facing messages in some cases which are localized) so we need t copy the entire dist directory and then pick the correct one in the build for auditorium.

See:

offen/script/gulpfile.js

Lines 43 to 44 in c7aed87

var dest = './dist/' + locale + '/'

var scriptTask = makeScriptTask(dest, locale)

and

offen/script/gulpfile.js

Lines 76 to 84 in c7aed87

return b

.plugin('tinyify')

.bundle()

.pipe(source('script.js'))

.pipe(buffer())

.pipe(gap.prependText('*/'))

.pipe(gap.prependFile('./../banner.txt'))

.pipe(gap.prependText('/**'))

.pipe(gulp.dest(dest))

To not bother you with more questions I'll need some time to work me through the build process. To be honest it is a lot magic for me right now.

I'm not quite sure how gulp works together with the dockerfile.build furthermore my overall gulp knowledge is very limited.

If you have questions while doing so, don't worry about bothering me. In any case, take your time. Thanks.

docker-compose.yml

lgtm-com · 2022-08-01T11:49:57Z

This pull request introduces 1 alert when merging 4d37805 into c7aed87 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

m90 · 2022-08-01T14:41:22Z

auditorium/gulpfile.js

+          return true
+        },
+        transform: transforms.map(function (transform) {
+          if (transform === '@offen/l10nify' || (Array.isArray(transform) && transform[0] === '@offen/l10nify')) {


It would be good if you could undo these formatting only changes as well even if standard is ok with them. It'll be easier to understand what changed if we don't have the additional noise in the diff.

Marcel Schepaniak and others added 4 commits July 26, 2022 11:18

Added cryptoJS to dependencies. Added placeholder for script inegrity…

f6d7558

… hash to docker-compose. Added UI&Integrity hash to audtiroum

cleanup

37a4691

WIP - copy script folder in dockerbuild

b3cc965

Merge branch 'offen:development' into development

a2f0344

raLaaaa mentioned this pull request Jul 28, 2022

Provide optional SRI hash when displaying embed code #565

Open

m90 reviewed Jul 28, 2022

View reviewed changes

raLaaaa marked this pull request as draft July 28, 2022 08:13

m90 reviewed Jul 28, 2022

View reviewed changes

Marcel Schepaniak added 3 commits July 29, 2022 08:22

added marker for translations as well as removed ternary class statement

4c95b87

Merge branch 'development' of github.com:raLaaaa/offen into development

e8f042f

copying index.js now works

ac23ced

m90 reviewed Jul 29, 2022

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

standarf formatted

4d37805

m90 reviewed Aug 1, 2022

View reviewed changes

Marcel Schepaniak added 3 commits August 2, 2022 11:50

WIP - formatting & placeholder value

4a99c27

WIP - added comments on what is not working right now

c330d71

switched hardcoded path to dist

928990a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#565 Provide optional SRI hash when displaying embed code #730

#565 Provide optional SRI hash when displaying embed code #730

raLaaaa commented Jul 28, 2022

m90 Jul 28, 2022

m90 Jul 28, 2022

m90 Jul 28, 2022

lgtm-com bot commented Jul 29, 2022

raLaaaa commented Jul 29, 2022 •

edited

m90 commented Jul 29, 2022

m90 Jul 29, 2022

raLaaaa Aug 1, 2022

m90 Aug 1, 2022

raLaaaa Aug 4, 2022

m90 Aug 4, 2022

lgtm-com bot commented Aug 1, 2022

m90 Aug 1, 2022

	var dest = './dist/' + locale + '/'
	var scriptTask = makeScriptTask(dest, locale)

	return b
	.plugin('tinyify')
	.bundle()
	.pipe(source('script.js'))
	.pipe(buffer())
	.pipe(gap.prependText('*/'))
	.pipe(gap.prependFile('./../banner.txt'))
	.pipe(gap.prependText('/**'))
	.pipe(gulp.dest(dest))

#565 Provide optional SRI hash when displaying embed code #730

Are you sure you want to change the base?

#565 Provide optional SRI hash when displaying embed code #730

Conversation

raLaaaa commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jul 29, 2022

raLaaaa commented Jul 29, 2022 • edited

m90 commented Jul 29, 2022

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

Choose a reason for hiding this comment

lgtm-com bot commented Aug 1, 2022

Choose a reason for hiding this comment

raLaaaa commented Jul 29, 2022 •

edited