Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve CI #201

Merged
merged 4 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
name: CI

on:
# Triggers the workflow on push or pull request events but only for the main branch
push:
branches: [ master ]
pull_request:
branches: [ master ]

# Allows you to run this workflow manually from the Actions tab
workflow_dispatch:

jobs:
verify_json:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4

- name: Install yajl-tools
run: sudo apt-get install -qq yajl-tools

- name: Check that JSON files are valid
run: for f in *.json dist/*.json filter_lists/*.json ; do json_verify < $f || echo "$f failed" ; done

confirm_built:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4

- name: Install node
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4
with:
node-version: "20"

- name: Get changed resource-related files
id: changed-files
uses: tj-actions/changed-files@cbda684547adc8c052d50711417fa61b428a9f88 # v41
with:
files: |
resources
metadata.json

- name: Build and verify
if: steps.changed-files.outputs.any_changed == 'true'
run: |
npm run build
npm run generateMetadata

- name: Check for any drift via git diff
if: steps.changed-files.outputs.any_changed == 'true'
run: |
git diff --no-ext-diff --exit-code metadata.json

check_format:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4

- name: Install node
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4
with:
node-version: "20"

- name: See if list catalog has changed
id: changed-files
uses: tj-actions/changed-files@cbda684547adc8c052d50711417fa61b428a9f88 # v41
with:
files: |
filter_lists/list_catalog.json

- name: Run format tests
if: steps.changed-files.outputs.any_changed == 'true'
run: |
npm run test
30 changes: 0 additions & 30 deletions .github/workflows/json_verify.yml

This file was deleted.

38 changes: 0 additions & 38 deletions .github/workflows/verify_metadata.yml

This file was deleted.

8 changes: 4 additions & 4 deletions build.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const fs = require('fs')
const path = require('path')
import fs from 'fs'
import path from 'path'

const { readResources } = require('.')
import { readResources } from './index.js'

fs.writeFileSync(path.join(__dirname, 'dist', 'resources.json'), JSON.stringify(readResources()))
fs.writeFileSync(path.join(import.meta.dirname, 'dist', 'resources.json'), JSON.stringify(readResources()))
4 changes: 2 additions & 2 deletions generateMetadataJsonFromScriptResources.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
* For instance - brave-fix script needs the "bf" alias
*/

const fs = require("fs");
const path = require("path");
import fs from "fs";
import path from "path";

const metadataJsonFile = "metadata.json";
const resourcesDir = "resources";
Expand Down
12 changes: 6 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
const fs = require('fs')
const path = require('path')
import fs from 'fs'
import path from 'path'

const metadata = require('./metadata.json')
import metadata from './metadata.json' with { type: "json" }

const readResources = (() => {
return metadata.map(item => ({
name: item.name,
aliases: item.aliases,
kind: item.kind,
content: fs.readFileSync(path.join(__dirname, 'resources', item.resourcePath)).toString('base64')
content: fs.readFileSync(path.join(import.meta.dirname, 'resources', item.resourcePath)).toString('base64')

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] Detected possible user input going into a path.join or path.resolve function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.

Source: https://semgrep.dev/r/javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal


Cc @thypon @bcaller

}))
})

const listCatalog = require('./filter_lists/list_catalog.json')
import listCatalog from './filter_lists/list_catalog.json' with { type: "json" }

module.exports = { listCatalog, readResources }
export { listCatalog, readResources }
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "1.0.0",
"description": "Custom resources and scriptlets used for Brave's adblocker",
"main": "index.js",
"type": "module",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

required for import and stuff

"scripts": {
"build": "node build",
"test": "node verify",
Expand Down
22 changes: 18 additions & 4 deletions verify.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
const { readResources, listCatalog } = require('.')
import { readResources, listCatalog } from './index.js'

const assert = require('node:assert')
const test = require('node:test')
const { Engine, FilterFormat, FilterSet } = require('adblock-rs')
import assert from 'node:assert'
import crypto from 'crypto'
import test from 'node:test'
import { Engine, FilterFormat, FilterSet } from 'adblock-rs'

const getIDFromBase64PublicKey = (key) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought you said we already have this code somewhere? Can we not just export that function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's deep in brave-core-crx-packager internals. I considered using it directly but it doesn't seem worth it to expose that and pull it in as a new dependency here just for CI purposes

const hash = crypto.createHash('sha256')
const data = Buffer.from(key, 'base64')
const digest = hash.update(data).digest('hex')
const id = digest.toString().substring(0, 32)
return id.replace(/[0-9a-f]/g, (c) => {
return 'abcdefghijklmnop'.charAt('0123456789abcdef'.indexOf(c))
})
}

test('resources are parsed OK by adblock-rust', t => {
const resources = readResources()
Expand Down Expand Up @@ -34,6 +45,9 @@ const testLists = (lists) => {
assert.ok(list.list_text_component.component_id !== undefined && typeof list.list_text_component.component_id === 'string')
assert.ok(list.list_text_component.base64_public_key !== undefined && typeof list.list_text_component.base64_public_key === 'string')

assert.ok(list.component_id === getIDFromBase64PublicKey(list.base64_public_key))
assert.ok(list.list_text_component.component_id === getIDFromBase64PublicKey(list.list_text_component.base64_public_key))

assert.ok(list.sources !== undefined && Array.isArray(list.sources))
for (const source of list.sources) {
assert.ok(source.url !== undefined && typeof source.url === 'string')
Expand Down