diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000000..0112663fb6 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,112 @@ +# Contributing + +## Workspace-specific guides + +- [Injected coding guidelines](./injected/docs/coding-guidelines.md) +- [Injected testing guide](./injected/docs/testing-guide.md) +- [Special Pages README](./special-pages/README.md) +- [Messaging docs](./messaging/docs/messaging.md) + +## Development + +Consider using [nvm](https://github.com/nvm-sh/nvm) to manage node versions, after installing in the project directory run: + +``` +nvm use +``` + +From the top-level root folder of this npm workspace, you can run the following npm commands: + +**Install dependencies**: + +Will install all the dependencies we need to build and run the project: +``` +npm install +``` + +**Build all workspaces**: + +Use this to produce the same output as a release. The `build` directory will be populated with +various artifacts. + +```sh +npm run build +``` + +> [!TIP] +> You can run the `build` command from within any sub-project too, the artifacts will always be +> lifted out to the root-level `build` folder. + +**Run unit tests for all workspaces**: + +```sh +npm run test-unit +``` + +**Run integration tests for all workspaces**: +```sh +npm run test-int +``` + +**Run extended integration tests for all workspaces**: +```sh +npm run test-int-x +``` + +**Clean tree and check for changes**: +```sh +npm run test-clean-tree +``` + +**Generate documentation using TypeDoc**: +```sh +npm run docs +``` + +**Generate and watch documentation using TypeDoc**: +```sh +npm run docs-watch +``` + +**Compile TypeScript files**: +```sh +npm run tsc +``` + +**Watch and compile TypeScript files**: +```sh +npm run tsc-watch +``` + +**Lint the codebase using ESLint**: +```sh +npm run lint +``` + +**Lint and automatically fix issues**: +```sh +npm run lint-fix +``` + +**Serve integration test pages on port 3220**: +```sh +npm run serve +``` + +**Serve special pages on port 3221**: +```sh +npm run serve-special-pages +``` + +## Release Process + +Releases are created via GitHub Actions: [Release workflow](https://github.com/duckduckgo/content-scope-scripts/actions/workflows/build.yml) + +### Creating a release + +1. Go to [Actions → Release](https://github.com/duckduckgo/content-scope-scripts/actions/workflows/build.yml) +2. Click "Run workflow" +3. Select version bump type +4. Click "Run workflow" + +The workflow creates a tag and GitHub release automatically. Build artifacts on the `releases` branch are consumed by native app repos. diff --git a/README.md b/README.md index a8b2625204..d5f8e2dfa2 100644 --- a/README.md +++ b/README.md @@ -30,93 +30,6 @@ Utilities to automatically generate TypeScript types from JSON Schema files. --- -## NPM commands +## Contributing -Consider using [nvm](https://github.com/nvm-sh/nvm) to manage node versions, after installing in the project directory run: - -``` -nvm use -``` - -From the top-level root folder of this npm workspace, you can run the following npm commands: - -**Install dependencies**: - -Will install all the dependencies we need to build and run the project: -``` -npm install -``` - -**Build all workspaces**: - -Use this to produce the same output as a release. The `build` directory will be populated with -various artifacts. - -```sh -npm run build -``` - -> [!TIP] -> You can run the `build` command from within any sub-project too, the artifacts will always be -> lifted out to the root-level `build` folder. - -**Run unit tests for all workspaces**: - -```sh -npm run test-unit -``` - -**Run integration tests for all workspaces**: -```sh -npm run test-int -``` - -**Run extended integration tests for all workspaces**: -```sh -npm run test-int-x -``` - -**Clean tree and check for changes**: -```sh -npm run test-clean-tree -``` - -**Generate documentation using TypeDoc**: -```sh -npm run docs -``` - -**Generate and watch documentation using TypeDoc**: -```sh -npm run docs-watch -``` - -**Compile TypeScript files**: -```sh -npm run tsc -``` - -**Watch and compile TypeScript files**: -```sh -npm run tsc-watch -``` - -**Lint the codebase using ESLint**: -```sh -npm run lint -``` - -**Lint and automatically fix issues**: -```sh -npm run lint-fix -``` - -**Serve integration test pages on port 3220**: -```sh -npm run serve -``` - -**Serve special pages on port 3221**: -```sh -npm run serve-special-pages -``` +See [CONTRIBUTING.md](./CONTRIBUTING.md) for development setup, npm commands, and release process. diff --git a/injected/.cursor/rules/injected.mdc b/injected/.cursor/rules/injected.mdc new file mode 100644 index 0000000000..2922e90fa7 --- /dev/null +++ b/injected/.cursor/rules/injected.mdc @@ -0,0 +1,70 @@ +--- +description: Guidelines for features running in web page DOM environments +globs: injected/** +--- + +# Injected Features + +JavaScript features injected into web pages or running in DOM environments of DuckDuckGo browsers. Features extend `ContentFeature` and `ConfigFeature` and integrate with remote configuration for per-site enable/disable. + +**See also:** +- `docs/coding-guidelines.md` for code style, patterns, and examples +- `docs/css-decision-guide.md` for when to use C-S-S vs native code +- [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc) for A/B testing features + +## Structure + +``` +src/features/.js # Feature implementation (extends ContentFeature) +src/features.js # Feature registry (baseFeatures + otherFeatures arrays) +entry-points/.js # Platform-specific bundles +integration-test/ # Playwright integration tests +unit-test/ # Jasmine unit tests +``` + +**Adding a new feature:** Add to `baseFeatures` or `otherFeatures` in `src/features.js`, then add to relevant platform arrays in `platformSupport`. + +## TypeScript + +JSDoc types in JavaScript files. Import types via `@typedef`: + +```javascript +/** @typedef {import('../types/feature.js').SettingType} SettingType */ +``` + +## Testing + +Run from `injected/` directory: + +| Command | Purpose | +|---------|---------| +| `npm run test-unit` | Jasmine unit tests | +| `npm run test-int` | Playwright integration tests | +| `npm run build` | Build all platform bundles | + +Integration tests use test pages in `integration-test/test-pages/`. + +## Key Patterns + +- **Event listeners**: Use `handleEvent` pattern or stored references (avoid `.bind(this)`) +- **API existence**: Check `typeof API === 'function'` before wrapping +- **DOM timing**: Check `document.readyState` before accessing DOM +- **Error types**: Match native error signatures in API shims + +See `docs/coding-guidelines.md` for DDGProxy, captured globals, retry utilities, and other patterns. + +## Platform Considerations + +Features are bundled per-platform in `entry-points/`: +- `apple.js` - iOS/macOS +- `android.js` - Android +- `windows.js` - Windows +- `extension-mv3.js` - Browser extension + +Platform-specific features: `navigatorInterface`, `windowsPermissionUsage`, `messageBridge`, `favicon` (see `utils.js` `platformSpecificFeatures`). + +## Notes + +- When running Playwright commands, use `--reporter list` to prevent hanging +- Features can be enabled/disabled per-site via remote config +- Add debug flags with `this.addDebugFlag()` for breakage reports diff --git a/injected/README.md b/injected/README.md index f99f0245fb..e157237ec8 100644 --- a/injected/README.md +++ b/injected/README.md @@ -4,7 +4,7 @@ Content Scope Scripts handles injecting DOM modifications in a browser context; ## Quick Start -Content Scope Scripts provides a unified API for browser privacy features across multiple platforms (Firefox, Chrome, Safari, Android, iOS). Features are loaded dynamically based on remote configuration and can be enabled/disabled per site. +Content Scope Scripts provides a unified API for features running in web page DOM environments across multiple platforms (Firefox, Chrome, Safari, Android, iOS). Features are loaded dynamically based on remote configuration and can be enabled/disabled per site. ## Documentation @@ -23,12 +23,12 @@ Content Scope Scripts provides a unified API for browser privacy features across Content Scope Scripts contains two main sub-projects: - **[Special Pages](../special-pages/)** - HTML/CSS/JS applications loaded into browsers (DuckPlayer, Release Notes, New Tab page, etc.) -- **Injected Features** - Features injected into websites (privacy protections, compatibility fixes) +- **Injected Features** - Features injected into websites (privacy protections, compatibility fixes, DOM manipulations) > **For Special Pages development**, see the [Special Pages README](../special-pages/README.md) for detailed getting started instructions. ### Features -Features are JavaScript modules that implement privacy protections. Each feature: +Features are JavaScript modules running in web page DOM environments. Each feature: - Extends the `ConfigFeature` class for remote configuration support - Implements the feature lifecycle (`load`, `init`, `update`) - Can be enabled/disabled per site via remote configuration diff --git a/injected/docs/README.md b/injected/docs/README.md index 802032362d..2a4fe2cb04 100644 --- a/injected/docs/README.md +++ b/injected/docs/README.md @@ -15,6 +15,7 @@ This directory contains detailed documentation for the Content Scope Scripts pro - **[Development Utilities](./development-utilities.md)** - Scope injection utilities and development tools - **[Testing Guide](./testing-guide.md)** - Local testing and development workflow +- **[Coding Guidelines](./coding-guidelines.md)** - Code style, architecture, security, and performance guidelines ### Existing Documentation @@ -25,7 +26,8 @@ This directory contains detailed documentation for the Content Scope Scripts pro If you're new to Content Scope Scripts, start with the main [README](../README.md) for a high-level overview, then dive into the specific documentation based on your needs: -- **Building features?** → [Features Guide](./features-guide.md) +- **Building features?** → [Features Guide](./features-guide.md) (feature files contain inline guidelines) +- **Writing code?** → [Coding Guidelines](./coding-guidelines.md) - **Integrating with a platform?** → [Platform Integration](./platform-integration.md) and [Platform-Specific Build & Troubleshooting Tips](./build-and-troubleshooting.md) - **Using the API?** → [API Reference](./api-reference.md) - **Developing utilities?** → [Development Utilities](./development-utilities.md) diff --git a/injected/docs/coding-guidelines.md b/injected/docs/coding-guidelines.md new file mode 100644 index 0000000000..2f500e27fc --- /dev/null +++ b/injected/docs/coding-guidelines.md @@ -0,0 +1,479 @@ +# Coding Guidelines + +> Guidelines for injected features development. + +## Feature Pattern + +Features extend `ContentFeature` (which itself extends `ConfigFeature`). Use `ContentFeature` for features that need messaging, logging, and DOM interaction. Implement lifecycle methods: + +```js +import ContentFeature from '../content-feature.js'; + +export default class MyFeature extends ContentFeature { + init() { + // Main initialization - feature is enabled for this site + if (this.getFeatureSettingEnabled('someSetting')) { + this.applySomeFix(); + } + } + + load() { + // Early load - before remote config (use sparingly) + } + + update(data) { + // Receive updates from browser + } +} +``` + +## Remote Configuration + +Use `getFeatureSetting()` and `getFeatureSettingEnabled()` to read config: + +```js +// Boolean check with default +if (this.getFeatureSettingEnabled('settingName')) { ... } +if (this.getFeatureSettingEnabled('settingName', 'disabled')) { ... } // default disabled + +// Get setting value (returns typed object from privacy-configuration schema) +const settings = this.getFeatureSetting('settingName'); +``` + +**Feature state values:** `"enabled"` or `"disabled"`. Features default to disabled unless explicitly enabled. + +Types are generated from `@duckduckgo/privacy-configuration/schema/features/.json`. + +### Conditional Changes + +Use `conditionalChanges` to apply JSON Patch operations based on runtime conditions. Conditions are evaluated in `src/config-feature.js` (see `ConditionBlock` typedef and `_matchConditionalBlock`). + +**Supported conditions:** + +| Condition | Description | Example | +| --------------------- | -------------------------- | ------------------------------------------------------------------- | +| `domain` | Match hostname | `"domain": "example.com"` | +| `urlPattern` | Match URL (URLPattern API) | `"urlPattern": "https://*.example.com/*"` | +| `experiment` | Match A/B test cohort | `"experiment": { "experimentName": "test", "cohort": "treatment" }` | +| `context` | Match frame type | `"context": { "frame": true }` or `"context": { "top": true }` | +| `minSupportedVersion` | Minimum platform version | `"minSupportedVersion": { "ios": "17.0" }` | +| `maxSupportedVersion` | Maximum platform version | `"maxSupportedVersion": { "ios": "18.0" }` | +| `injectName` | Match inject context | `"injectName": "apple-isolated"` | +| `internal` | Internal builds only | `"internal": true` | +| `preview` | Preview builds only | `"preview": true` | + +**Config example:** + +```json +{ + "settings": { + "conditionalChanges": [ + { + "condition": { "domain": "example.com" }, + "patchSettings": [{ "op": "replace", "path": "/someSetting", "value": true }] + }, + { + "condition": [{ "urlPattern": "https://site1.com/*" }, { "urlPattern": "https://site2.com/path/*" }], + "patchSettings": [{ "op": "add", "path": "/newSetting", "value": "enabled" }] + } + ] + } +} +``` + +**Key rules:** + +- All conditions in a block must match (AND logic) +- Array of condition blocks uses OR logic (any block matching applies the patch) +- `patchSettings` uses [RFC 6902 JSON Patch](https://datatracker.ietf.org/doc/html/rfc6902) with [RFC 6901 JSON Pointer](https://datatracker.ietf.org/doc/html/rfc6901) paths (`/setting/nested`) +- Unsupported conditions cause the block to fail (for backwards compatibility) + +For A/B testing, see [privacy-configuration experiments guide](https://github.com/duckduckgo/privacy-configuration/blob/main/.cursor/rules/content-scope-experiments.mdc). + +## Messaging + +Use inherited messaging methods: + +```js +// Fire-and-forget +this.notify('messageName', { data }); + +// Request/response +const response = await this.request('messageName', { data }); + +// Subscribe to updates +this.subscribe('eventName', (data) => { ... }); +``` + +## API Shims & Error Types + +When shimming browser APIs, use the correct error types to match native behavior: + +```js +// TypeError for invalid arguments +throw new TypeError("Failed to execute 'lock' on 'ScreenOrientation': 1 argument required"); + +// DOMException with name for API-specific errors +throw new DOMException('Share already in progress', 'InvalidStateError'); +throw new DOMException('Permission denied', 'NotAllowedError'); +return Promise.reject(new DOMException('No device selected.', 'NotFoundError')); +``` + +Common DOMException names: `InvalidStateError`, `NotAllowedError`, `NotFoundError`, `AbortError`, `DataError`, `SecurityError`. + +## Code Style + +### Constants + +Avoid constants in the code and prefer using `this.getFeatureSetting('constantName') ?? defaultValue` to allow for remote configuration to modify the value. + +When using `getFeatureSettingEnabled()`, use its built-in default parameter rather than `|| true`: + +```js +// ✅ Correct - uses second parameter for default +includeIframes: this.getFeatureSettingEnabled('includeIframes', 'enabled'); + +// ❌ Wrong - || true ignores explicit false from config +includeIframes: this.getFeatureSettingEnabled('includeIframes') || true; +``` + +### Event Listener Management + +Use stored references or the class-based `handleEvent` pattern to ensure proper removal: + +#### Stored reference pattern: + +```js +this.scrollListener = () => {...}; +document.addEventListener('scroll', this.scrollListener); +document.removeEventListener('scroll', this.scrollListener); +``` + +#### Class-based handleEvent pattern: + +```js +class MyFeature extends ContentFeature { + init() { + document.addEventListener('scroll', this); + } + destroy() { + document.removeEventListener('scroll', this); + } + handleEvent(e) { + if (e.type === 'scroll') { + requestAnimationFrame(() => this.updatePosition()); + } + } +} +``` + +**Avoid** using `.bind(this)` directly in addEventListener—it creates a new reference each time, preventing removal. + +### Module Structure + +- Extract reusable logic into separate files for focused unit testing +- Security-sensitive operations (e.g., markdown to HTML conversion) should be isolated with extensive tests + +### Error Handling and Debugging + +- Avoid hardcoding debug flags; ensure they are configurable and environment-dependent +- Remove `console.log` statements from production code and prefer `this.log.info` instead as this will be disabled in release. + +## Architecture & Design + +### Action Execution Flow + +- Execute secondary actions from the top level to avoid context handling issues +- Avoid re-calling execution functions within an action + +### Navigation Event Handling + +- Use `navigatesuccess` event for URL change detection (ensures navigation is committed): + +```js +globalThis.navigation.addEventListener('navigatesuccess', handleURLChange); +``` + +### URL Change Lifecycle + +Enable URL tracking for features that need to respond to SPA navigation: + +```js +export default class MyFeature extends ContentFeature { + listenForUrlChanges = true; // Enable URL change tracking + + init() { + this.applyFeature(); + } + + urlChanged(navigationType) { + // Called automatically on URL changes + this.recomputeSiteObject(); // Update config for new path + this.applyFeature(); + } +} +``` + +Navigation types: `'push'`, `'replace'`, `'traverse'`, `'reload'`. + +### Re-entrancy Pattern + +- Check `document.readyState` to avoid missing DOM elements: + +```js +if (document.readyState === 'loading') { + document.addEventListener('DOMContentLoaded', () => this.applyFix()); +} else { + this.applyFix(); +} +``` + +### DDGProxy Pattern + +Use `DDGProxy` for wrapping browser APIs safely. It automatically adds debug flags, checks exemptions, and preserves `toString()` behavior: + +```js +import { DDGProxy, DDGReflect } from '../utils'; + +// Wrap a method +const proxy = new DDGProxy(this, Navigator.prototype, 'getBattery', { + apply(target, thisArg, args) { + return Promise.reject(new DOMException('Not allowed', 'NotAllowedError')); + }, +}); +proxy.overload(); + +// Wrap a property getter +const propProxy = new DDGProxy(this, Screen.prototype, 'width', { + get(target, prop, receiver) { + return 1920; + }, +}); +propProxy.overloadProperty(); +``` + +### Retry Utilities + +Use built-in retry utilities for operations that may need multiple attempts: + +```js +import { retry, withRetry } from '../timer-utils'; + +// Simple retry with config (returns { result, exceptions } for debugging) +const { result, exceptions } = await retry(() => findElement(selector), { interval: { ms: 1000 }, maxAttempts: 30 }); + +// Retry with automatic error handling +const element = await withRetry( + () => document.querySelector(selector), + 4, // maxAttempts + 500, // delay ms + 'exponential', // strategy: 'linear' | 'exponential' +); +``` + +## Security & Privacy + +### Element Validation + +- Validate elements and their types before operations: + +```js +if (!element) { + return; // or throw appropriate error +} +if (element instanceof HTMLInputElement || element instanceof HTMLTextAreaElement) { + element.value = value; +} +``` + +### Caching and State Management + +- Avoid global or static caching mechanisms that could lead to race conditions +- Use instance-scoped storage or include all relevant identifiers in cache keys + +### Permissions Handling + +- Ensure custom permission handling does not bypass native permission models +- Handle permissions with custom behaviors or name overrides correctly + +### API Usage in Iframes + +- Be cautious enabling APIs like Web Share within iframes, understand that you're exposing message overhead and potential side effects to a third party. + +### Captured Globals Pattern + +Use captured globals to avoid page-tampered native APIs: + +```js +import * as capturedGlobals from '../captured-globals.js'; + +// Use captured versions instead of global +const myMap = new capturedGlobals.Map(); +const mySet = new capturedGlobals.Set(); + +// Dispatch events safely +capturedGlobals.dispatchEvent(new capturedGlobals.CustomEvent('name', { detail })); +``` + +### Frame and Context Guards + +Validate execution context at feature initialization: + +```js +import { isBeingFramed } from '../utils'; + +init(args) { + if (isBeingFramed()) return; // Skip if in a frame + if (!isSecureContext) return; // Skip if not HTTPS + if (!args.messageSecret) return; // Skip if missing required args +} +``` + +### Message Secret Pattern + +For cross-world communication, use message secrets to prevent spoofing: + +```js +function appendToken(eventName) { + return `${eventName}-${args.messageSecret}`; +} + +// Listen with token +captured.addEventListener(appendToken('MessageType'), handler); + +// Dispatch with token +const event = new captured.CustomEvent(appendToken('Response'), { detail: payload }); +captured.dispatchEvent(event); +``` + +## Performance + +### Memory Management with WeakCollections + +Use `WeakSet`/`WeakMap` for DOM element references to allow garbage collection: + +```js +const elementCache = new WeakMap(); + +function getOrCompute(element) { + if (elementCache.has(element)) { + return elementCache.get(element); + } + const result = expensiveComputation(element); + elementCache.set(element, result); + return result; +} + +// Delete entries from regular collections after use +navigations.delete(event.target); +``` + +### Timed Execution Strategy + +For dynamic content, use multiple passes at staggered intervals: + +```js +const hideTimeouts = [0, 100, 300, 500, 1000, 2000, 3000]; +const unhideTimeouts = [1250, 2250, 3000]; + +hideTimeouts.forEach((timeout) => { + setTimeout(() => hideAdNodes(rules), timeout); +}); + +// Clear caches after all operations complete +const clearCacheTimer = Math.max(...hideTimeouts, ...unhideTimeouts) + 100; +setTimeout(() => { + appliedRules = new Set(); + hiddenElements = new WeakMap(); +}, clearCacheTimer); +``` + +Note: Timers are a useful heuristic to save resources but should be remotely configurable and often other techniques such as carefully engineered MutationObservers would be preferred. + +### Batch DOM Operations + +Use semaphores to batch frequent DOM updates: + +```js +let updatePending = false; + +function scheduleUpdate() { + if (!updatePending) { + updatePending = true; + setTimeout(() => { + performDOMUpdate(); + updatePending = false; + }, 10); + } +} +``` + +### Message Bridge Caching + +- Avoid global or static caches for message bridges +- Include all relevant parameters (`featureName`, `messageSecret`) in cache keys + +## Error Handling + +### Async/Await Usage + +- Use `await` to ensure errors are caught and flow is maintained: + +```js +await someAsyncFunction(); +``` + +### Error Handling in Promises + +- Ensure promises have both resolve and reject paths: + +```js +new Promise((resolve, reject) => { + if (condition) { + resolve(result); + } else { + reject(new Error('specific error message')); + } +}); +``` + +### Null Checks + +- Perform null checks before using objects: + +```js +if (object != null) { + // Use the object +} +``` + +### Structured Error Responses + +Use typed error classes for action-based features: + +```js +import { ErrorResponse } from './broker-protection/types.js'; + +const response = new ErrorResponse({ + actionID: action.id, + message: 'Descriptive error message', +}); +this.messaging.notify('actionError', { error: response }); +``` + +### Silent Feature Initialization Failures + +Catch errors without breaking other features: + +```js +try { + customElements.define('ddg-element', DDGElement); +} catch (e) { + // May fail on extension reload or conflicts + console.error('Custom element definition failed:', e); +} +``` + +## Testing + +See [Testing Guide](./testing-guide.md) for comprehensive testing documentation. diff --git a/injected/docs/css-decision-guide.md b/injected/docs/css-decision-guide.md new file mode 100644 index 0000000000..a8c5a1d1a9 --- /dev/null +++ b/injected/docs/css-decision-guide.md @@ -0,0 +1,42 @@ +# When to Use C-S-S + +Decision guide for when to use C-S-S injected scripts vs native code. + +## C-S-S Injected Scripts + +Best for: + +- In-page changes or DOM manipulations +- Anything modifying a DOM API or manipulating page content +- Code that benefits from config, breakage tooling, and DOM wrappers +- Testing/linting/bundling makes code more robust than native-only + +## Native Code + +Best for: + +- Listening to bridge/handler messages +- When WebView APIs do the job better +- Avoid injecting scripts when native can handle it—harder to debug, no tooling + +## SERP/Static Pages + +Can expose/call bridge messages, cleaner than scraping page content. + +## Decision Checklist + +| Question | Recommendation | +| --------------------------------------- | ----------------------------------------------------- | +| Modifying DOM API or manipulating page? | **Must** be in C-S-S | +| Need config handling, testing? | Prefer C-S-S (free tooling) | +| Code relies on page structure? | Use config to alter behavior; avoid if possible | +| Internal page involved? | Consider having the page expose an API/fire a message | +| Writing conditionals for iframe/domain? | Use `conditionalChanges` in config | +| Exposing new APIs? | Check if `apiManipulation` feature handles it | +| Need isolated context? | Prefer isolated entry point if possible | + +## Message Handlers vs Bridge + +- **Message handlers**: Limit to pages that need them; use script evaluation over messaging unless payload size is problematic +- **Bridge**: Config-enabled messages that pass through C-S-S; use subscriptions for exposing custom behavior to native +- If our site is involved, push messages to native rather than scraping diff --git a/injected/docs/testing-guide.md b/injected/docs/testing-guide.md index 688df48c2c..1aa4c8f3eb 100644 --- a/injected/docs/testing-guide.md +++ b/injected/docs/testing-guide.md @@ -53,9 +53,9 @@ npm run build ## Test Builds for Ship Review -Test builds are created with a GitHub workflow. The assets for Content Scope Scripts will be created on demand if they are absent (which they will be, if you're pointing to a branch of CSS). +Test builds are created with a GitHub workflow. The assets for Content Scope Scripts will be created on demand if they are absent (which they will be, if you're pointing to a branch of C-S-S). -1. Commit any changes to CSS and push a branch to the remote +1. Commit any changes to C-S-S and push a branch to the remote 2. Make sure you commit the submodule reference update in the Windows PR 3. Continue with "Build an installer for ship review / test" @@ -65,6 +65,123 @@ Test builds are created with a GitHub workflow. The assets for Content Scope Scr If you drop a `debugger;` line in the scripts and open DevTools window, the DevTools will breakpoint and navigate to that exact line in code when the debug point has been hit. -### Verifying CSS is Loaded +### Verifying C-S-S is Loaded Open DevTools, go to the Console tab and enter `navigator.duckduckgo`. If it's defined, then Content Scope Scripts is running. + +## Testing Best Practices + +### Test Independence + +Playwright tests must be independent for safe parallel execution: + +```javascript +// ✅ Each test is self-contained +test('overlay displays correctly', async ({ page }) => { + const overlays = OverlaysPage.create(page); + await overlays.openPage(url); + await expect(overlays.element).toBeVisible(); +}); + +// ❌ Avoid shared state between tests +let sharedPage; // Don't do this +``` + +Use Playwright's [test fixtures](https://playwright.dev/docs/test-fixtures) for complex setup. + +### Async/Await + +Always `await` async operations—missing `await` causes flaky tests: + +```javascript +// ✅ Correct +await overlays.opensShort(url); + +// ❌ Test passes incorrectly (promise not awaited) +overlays.opensShort(url); +``` + +### Glob Patterns + +Use specific patterns to avoid including non-test files: + +```javascript +// ✅ Correct - only .spec.js files +'integration-test/**/*.spec.js'; + +// ❌ Incorrect - includes config, fixtures, etc. +'integration-test/**'; +``` + +### Feature Flag Testing + +Match test setup to the feature state being tested: + +```javascript +// Testing disabled state +test('feature disabled shows fallback', async ({ page }) => { + await page.addInitScript(() => { + window.__ddg_config__ = { features: { myFeature: { state: 'disabled' } } }; + }); + // ... +}); +``` + +### Security-Sensitive Code + +Extract DOM manipulation and HTML generation to separate files for focused unit testing. This prevents XSS vulnerabilities from slipping through integration-only testing. + +## Integration Test Structure + +### Test Page Directory Layout + +``` +integration-test/ +├── test-pages/ +│ └── / +│ ├── config/ +│ │ └── config.json # Feature config fixtures +│ ├── pages/ +│ │ └── test-page.html # Test HTML pages +│ └── .spec.js # Playwright tests +``` + +### Config Fixture Pattern + +Create config fixtures that match privacy-configuration schema: + +```json +{ + "features": { + "featureName": { + "state": "enabled", + "settings": { + "settingName": "value", + "conditionalChanges": [ + { + "condition": { "domain": "localhost" }, + "patchSettings": [{ "op": "replace", "path": "/settingName", "value": "testValue" }] + } + ] + } + } + } +} +``` + +### Playwright Test Pattern + +```javascript +test('feature behavior', async ({ page }) => { + await page.goto('/test-pages/feature/pages/test.html'); + + // Wait for feature initialization + await page.waitForFunction(() => window.__ddg_feature_ready__); + + // Test feature behavior + const result = await page.evaluate(() => someAPI()); + expect(result).toBe(expectedValue); +}); +``` + +For conditional changes and config schema details, see the [injected cursor rules](../.cursor/rules/injected.mdc#conditional-changes). diff --git a/injected/src/features/autofill-import.js b/injected/src/features/autofill-import.js index 1b5e3513d2..763d96b720 100644 --- a/injected/src/features/autofill-import.js +++ b/injected/src/features/autofill-import.js @@ -59,7 +59,7 @@ export default class AutofillImport extends ActionExecutorBase { #isBookmarkModalVisible = false; - /** @type {WeakSet} */ + /** @type {WeakSet} Use WeakSet for DOM refs to allow GC */ #tappedElements = new WeakSet(); /** @@ -430,6 +430,8 @@ export default class AutofillImport extends ActionExecutorBase { } /** + * Event handler using class instance pattern. + * Use `addEventListener('scroll', this)` not `.bind(this)` to allow removal. * @param {Event} event */ handleEvent(event) { @@ -451,6 +453,7 @@ export default class AutofillImport extends ActionExecutorBase { /** * Checks if the path is supported for animation. + * NOTE: All paths here must have corresponding handlers in getElementAndStyleFromPath() * @param {string} path * @returns {boolean} */ diff --git a/injected/src/features/broker-protection/broker-protection.md b/injected/src/features/broker-protection/broker-protection.md index 9f92289046..4d53bed73e 100644 --- a/injected/src/features/broker-protection/broker-protection.md +++ b/injected/src/features/broker-protection/broker-protection.md @@ -19,3 +19,33 @@ sequenceDiagram Note over N: Some time passes... N->>W: 📩 onActionReceived(action, data) ``` + +## Error Handling + +Use `PirError` for typed errors throughout the broker-protection feature: + +```js +import { PirError } from './types.js'; + +// Creating errors +if (!element) { + return PirError.create('could not find element'); +} + +// Checking for errors +if (PirError.isError(result)) { + return createError(result.error.message); +} + +// Element validation pattern +if ((isInputElement(element) && ['text', 'hidden'].includes(element.type)) || isTextAreaElement(element)) { + element.value = token; +} else { + return PirError.create('element is neither a text input nor textarea'); +} +``` + +**Key principles:** +- Always bubble up errors to avoid silent failures +- Use specific error messages that identify the failing operation +- Check `PirError.isError()` before accessing result values diff --git a/injected/src/features/click-to-load.js b/injected/src/features/click-to-load.js index 2fe065ac92..f03ef22e7e 100644 --- a/injected/src/features/click-to-load.js +++ b/injected/src/features/click-to-load.js @@ -1,3 +1,12 @@ +/** + * @file Click to Load Feature + * @see injected/docs/coding-guidelines.md for general patterns + * + * Key reminders: + * - Don't set `inset: 'auto'` after copying positioning styles + * - Use `{ key: value }` message payloads, not primitives + * - Handler name is `contentScopeScripts` (not `contentScopeScriptsIsolated`) + */ import { Messaging, TestTransportConfig, WebkitMessagingConfig } from '../../../messaging/index.js'; import { createCustomEvent, originalWindowDispatchEvent } from '../utils.js'; import { logoImg, loadingImages, closeIcon, facebookLogo } from './click-to-load/ctl-assets.js'; diff --git a/injected/src/features/page-context.js b/injected/src/features/page-context.js index 8d375b49f0..6f9a586b60 100644 --- a/injected/src/features/page-context.js +++ b/injected/src/features/page-context.js @@ -1,3 +1,7 @@ +/** + * @file Page Context Feature + * @see injected/docs/coding-guidelines.md for general patterns + */ import ContentFeature from '../content-feature.js'; import { getFaviconList } from './favicon.js'; import { isDuckAi, isBeingFramed, getTabUrl } from '../utils.js'; @@ -179,6 +183,7 @@ export function domToMarkdown(node, settings, depth = 0) { } /** + * Safely get attribute or empty string to avoid null in markdown output * @param {Element} node * @param {string} attr * @returns {string} @@ -192,15 +197,17 @@ function collapseAndTrim(str) { return collapseWhitespace(str).trim(); } +/** + * Note: The whitespace difference between href/non-href cases is intentional. + * With href: collapse AND trim (for clean markdown links) + * Without href: collapse only (retain surrounding space context) + */ function getLinkText(node, children, settings) { const href = node.getAttribute('href'); const trimmedContent = collapseAndTrim(children); if (settings.trimBlankLinks && trimmedContent.length === 0) { return ''; } - // The difference in whitespace handling is intentional here. - // Where we don't wrap in a link: - // we should retain at least one preceding and following space. return href ? `[${trimmedContent}](${href})` : collapseWhitespace(children); } @@ -375,6 +382,8 @@ export default class PageContext extends ContentFeature { this.recheckCount++; this.invalidateCache(); + // Note: Pass false to handleContentCollectionRequest to prevent + // resetRecheckCount, avoiding unintended recheck loops this.handleContentCollectionRequest(false); }, delayMs); } @@ -445,6 +454,8 @@ export default class PageContext extends ContentFeature { } // Cache the result - setter handles timestamp and observer + // Note: We only cache if content exists. Consider caching empty content too + // if mutation observation is needed for initially empty pages. if (content.content.length > 0) { this.cachedContent = content; } diff --git a/injected/src/features/web-compat.js b/injected/src/features/web-compat.js index 719065e566..17694378e2 100644 --- a/injected/src/features/web-compat.js +++ b/injected/src/features/web-compat.js @@ -1,10 +1,15 @@ +/** + * @file Web Compatibility Feature + * @see injected/docs/coding-guidelines.md for general patterns + */ import ContentFeature from '../content-feature.js'; // eslint-disable-next-line no-redeclare import { URL } from '../captured-globals.js'; import { DDGProxy, DDGReflect } from '../utils'; import { wrapToString, wrapFunction } from '../wrapper-utils.js'; /** - * Fixes incorrect sizing value for outerHeight and outerWidth + * Fixes incorrect sizing value for outerHeight and outerWidth. + * Note: Avoid hardcoding window geometry values - use calculations or config where possible. */ function windowSizingFix() { if (window.outerHeight !== 0 && window.outerWidth !== 0) { @@ -167,7 +172,10 @@ export class WebCompat extends ContentFeature { } } - /** Shim Web Share API in Android WebView */ + /** + * Shim Web Share API in Android WebView + * Note: Always verify API existence before shimming + */ shimWebShare() { if (typeof navigator.canShare === 'function' || typeof navigator.share === 'function') return; @@ -341,7 +349,7 @@ export class WebCompat extends ContentFeature { try { const result = await nativeRequest('requestPermission', {}); const resultPermission = /** @type {NotificationPermission} */ (result?.permission || 'denied'); - // Update cached permission so Notification.permission reflects the new state + // Update cached permission so Notification.permission getter reflects new state permission = resultPermission; if (deprecatedCallback) { deprecatedCallback(resultPermission); @@ -393,9 +401,9 @@ export class WebCompat extends ContentFeature { return; } nativeNotify('closeNotification', { id: this.#id }); - // Remove from map first to prevent duplicate onclose from native event + // Remove from map BEFORE firing onclose to prevent duplicate events from native feature.#webNotifications.delete(this.#id); - // Fire onclose handler + // Fire onclose handler after cleanup if (typeof this.onclose === 'function') { try { // @ts-expect-error - NotificationPolyfill doesn't fully implement Notification interface @@ -535,14 +543,17 @@ export class WebCompat extends ContentFeature { "Failed to execute 'query' on 'Permissions': Failed to read the 'name' property from 'PermissionDescriptor': Required member is undefined.", ); } + // Don't bypass - all supportedPermissions need handling, including non-native with name overrides if (!settings.supportedPermissions || !(query.name in settings.supportedPermissions)) { throw new TypeError( `Failed to execute 'query' on 'Permissions': Failed to read the 'name' property from 'PermissionDescriptor': The provided value '${query.name}' is not a valid enum value of type PermissionName.`, ); } const permSetting = settings.supportedPermissions[query.name]; + // Use custom permission name if configured, else original query name const returnName = permSetting.name || query.name; let returnStatus = settings.permissionResponse || 'prompt'; + // Only query native for permissions marked native:true if (permSetting.native) { try { const response = await this.messaging.request(MSG_PERMISSIONS_QUERY, query); @@ -566,6 +577,7 @@ export class WebCompat extends ContentFeature { /** * Fixes screen lock/unlock APIs for Android WebView. + * Uses wrapProperty to match original property descriptors. */ screenLockFix() { const validOrientations = [ @@ -869,19 +881,22 @@ export class WebCompat extends ContentFeature { // This should never occur, but keeps typescript happy if (!settings) return; + // Handler strategies: 'reflect' (pass through), 'undefined' (hide), 'polyfill' (stub) const proxy = new Proxy(globalThis.webkit.messageHandlers, { get(target, messageName, receiver) { const handlerName = String(messageName); - // handle known message names, such as DDG webkit messaging + // 'reflect': pass through to real handler (e.g., DDG webkit messaging) if (settings.handlerStrategies.reflect.includes(handlerName)) { return Reflect.get(target, messageName, receiver); } + // 'undefined': hide handler existence if (settings.handlerStrategies.undefined.includes(handlerName)) { return undefined; } + // 'polyfill': return stub (use '*' for catch-all) if (settings.handlerStrategies.polyfill.includes('*') || settings.handlerStrategies.polyfill.includes(handlerName)) { return { postMessage() { @@ -901,12 +916,14 @@ export class WebCompat extends ContentFeature { } viewportWidthFix() { + // Note: This guard is intentional for legacy mode. onUserPreferencesMerged() calls + // this without the guard for cases where re-application is needed. if (this._viewportWidthFixApplied) { return; } this._viewportWidthFixApplied = true; + // Re-entrancy pattern: check readyState to avoid missing DOM elements if (document.readyState === 'loading') { - // if the document is not ready, we may miss the original viewport tag document.addEventListener('DOMContentLoaded', () => this.viewportWidthFixInner()); } else { this.viewportWidthFixInner(); @@ -933,7 +950,7 @@ export class WebCompat extends ContentFeature { viewportWidthFixInner() { /** @type {NodeListOf} **/ const viewportTags = document.querySelectorAll('meta[name=viewport i]'); - // Chrome respects only the last viewport tag + // Chrome respects only the last viewport tag - modify existing rather than adding new const viewportTag = viewportTags.length === 0 ? null : viewportTags[viewportTags.length - 1]; const viewportContent = viewportTag?.getAttribute('content') || ''; /** @type {readonly string[]} **/ diff --git a/netlify.toml b/netlify.toml index e8b6b7c465..944ade142a 100644 --- a/netlify.toml +++ b/netlify.toml @@ -1,2 +1,4 @@ [build] +command = "npm ci && npm run docs-netlify" +publish = "docs" ignore = "git log -1 --pretty=%B | grep dependabot" diff --git a/package.json b/package.json index 24f2c36f83..339b3c6467 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "test-int-snapshots-update": "npm run test-int-snapshots-update --workspaces --if-present", "test-clean-tree": "npm run build && sh scripts/check-for-changes.sh", "docs": "typedoc", + "docs-netlify": "npm run build-locales -w injected && npm run build -w special-pages && npm run docs && cp -R build/integration/ docs/build", "docs-watch": "typedoc --watch", "tsc": "tsc", "tsc-watch": "tsc --watch", diff --git a/special-pages/.cursorrules b/special-pages/.cursorrules index 8597f6543e..46a689b9ce 100644 --- a/special-pages/.cursorrules +++ b/special-pages/.cursorrules @@ -2,6 +2,8 @@ Preact-based HTML/CSS/JS applications embedded in DuckDuckGo browsers. Each page is an isolated app with privileged native API access. +**See also:** `docs/coding-guidelines.md` for code style and best practices. + ## Structure ```