add flaresolverr support - #38
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds optional FlareSolverr support to bypass Cloudflare protection when scraping Letterboxd. The implementation introduces a centralized HTTP client abstraction that conditionally routes requests through FlareSolverr or directly, based on configuration.
Changes:
- Added new
http-clientmodule with conditional FlareSolverr routing and comprehensive test coverage - Updated all scrapers (popular, movie, list, collections) to use the new HTTP client instead of direct fetch calls
- Added environment variables for FlareSolverr configuration with validation (URL, timeout, session)
- Updated integration test infrastructure with Docker-based FlareSolverr setup script
- Enhanced documentation with FlareSolverr usage examples and configuration details
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/http-client.ts | New HTTP client module with FlareSolverr integration and direct fetch fallback |
| src/util/http-client.test.ts | Comprehensive unit tests for HTTP client with mocked FlareSolverr responses |
| src/util/env.ts | Added FlareSolverr environment variables with Zod validation (URL, timeout, session) |
| src/scraper/popular.ts | Updated to use fetchHtml from http-client instead of direct fetch |
| src/scraper/movie.ts | Updated to use fetchHtml from http-client instead of direct fetch |
| src/scraper/list.ts | Updated to use fetchHtml from http-client instead of direct fetch |
| src/scraper/collections.ts | Updated to use fetchHtml from http-client instead of direct fetch |
| src/scraper/popular.test.ts | Updated mocks to use http-client module instead of global fetch |
| src/scraper/movie.test.ts | Updated mocks to use http-client module instead of global fetch |
| src/scraper/list.test.ts | Updated mocks to use http-client module instead of global fetch |
| src/scraper/collections.test.ts | Updated mocks to use http-client module instead of global fetch |
| scripts/run-integration-tests.sh | New script to start FlareSolverr container and run integration tests |
| jest.setup.js | Added FLARESOLVERR_URL environment variable for integration tests |
| package.json | Updated test:integration command to use new shell script |
| README.md | Added comprehensive FlareSolverr documentation with Docker Compose example |
| .env.example | Added FlareSolverr configuration examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface FlareSolverrResponse { | ||
| status: string; | ||
| message: string; | ||
| solution: { | ||
| url: string; | ||
| status: number; | ||
| response: string; | ||
| cookies: Array<{ name: string; value: string; domain: string }>; | ||
| userAgent: string; | ||
| }; | ||
| } |
There was a problem hiding this comment.
The FlareSolverrResponse interface defines solution as a non-nullable object, but when status is 'error', the solution field can be null (as demonstrated in the test at line 195 of http-client.test.ts). This type mismatch could lead to runtime errors when accessing data.solution.response on line 91. Consider making the solution field nullable: solution: { ... } | null; and add a null check before accessing solution properties.
| // Unit tests mock the http-client module so this won't affect them | ||
| process.env.FLARESOLVERR_URL = 'http://localhost:8191/v1'; |
There was a problem hiding this comment.
Setting FLARESOLVERR_URL globally in jest.setup.js will cause all unit tests to use FlareSolverr by default, which is not the intended behavior. Unit tests that mock the http-client module will be unaffected, but any tests that don't mock it will attempt to connect to FlareSolverr. This could cause unit tests to fail if FlareSolverr is not running or if the env module is imported before mocking. Consider only setting this environment variable for integration tests, or document that unit tests must mock the http-client module to avoid this behavior.
| // Unit tests mock the http-client module so this won't affect them | |
| process.env.FLARESOLVERR_URL = 'http://localhost:8191/v1'; | |
| // Only set this for integration test runs to avoid affecting unit tests. | |
| // Respect any pre-existing FLARESOLVERR_URL provided by the environment. | |
| if (process.env.INTEGRATION_TESTS === 'true' && !process.env.FLARESOLVERR_URL) { | |
| process.env.FLARESOLVERR_URL = 'http://localhost:8191/v1'; | |
| } |
| docker stop $CONTAINER_NAME 2>/dev/null || true | ||
| docker rm $CONTAINER_NAME 2>/dev/null || true |
There was a problem hiding this comment.
The variable references in the shell script should be quoted to prevent word splitting and globbing issues. Lines 9, 10, 17, and elsewhere use unquoted variables like $CONTAINER_NAME. While this will work in most cases, it's a best practice to quote variables in shell scripts to avoid potential issues if the variable contains spaces or special characters. Consider using "$CONTAINER_NAME" instead of $CONTAINER_NAME throughout the script.
| ports: | ||
| - "8191:8191" |
There was a problem hiding this comment.
The Docker Compose example exposes the flaresolverr API on the host with the "8191:8191" port mapping, which makes this powerful headless-browser proxy reachable by any host that can reach the Docker engine. An attacker with network access could send arbitrary requests through FlareSolverr (SSRF) to scan or access internal HTTP services or abuse the browser, even though it should only be called by lettarrboxd. Restrict this service so it is not exposed on all interfaces (for example, bind only to 127.0.0.1 or avoid a host port mapping and keep access limited to the internal Docker network).
| ports: | |
| - "8191:8191" |
No description provided.