Skip to content
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
34 changes: 32 additions & 2 deletions app/web/api/v1/create_feed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,46 @@ def build_create_params(params, account)
# @param account [Hash]
# @return [String]
def validated_url(raw_url, account)
url = raw_url.to_s.strip
url = normalized_input_url(raw_url)
raise Html2rss::Web::BadRequestError, 'URL parameter is required' if url.empty?
raise Html2rss::Web::BadRequestError, 'Invalid URL format' unless UrlValidator.valid_url?(url)

url = UrlValidator.canonical_url(url)
raise Html2rss::Web::BadRequestError, 'Invalid URL format' unless url
unless UrlValidator.url_allowed?(account, url)
raise Html2rss::Web::ForbiddenError, 'URL not allowed for this account'
end

url
end

# @param raw_url [String, nil]
# @return [String]
def normalized_input_url(raw_url)
url = raw_url.to_s.strip
return url if url.empty?
return "https:#{url}" if url.start_with?('//')
return url if absolute_url?(url)

hostname_input?(url) ? "https://#{url}" : url
end

# @param url [String]
# @return [Boolean]
def absolute_url?(url)
url.match?(%r{\A[a-z][a-z0-9+\-.]*://}i)
end

# @param url [String]
# @return [Boolean]
def hostname_input?(url)
%r{
\A
(localhost(?::\d+)?|(?:\d{1,3}\.){3}\d{1,3}(?::\d+)?|(?:[a-z0-9-]+\.)+[a-z]{2,}(?::\d+)?)
(?:[/?#].*)?
\z
}ix.match?(url)
end

# @param raw_strategy [String, nil]
# @return [String]
def normalize_strategy(raw_strategy)
Expand Down
12 changes: 9 additions & 3 deletions app/web/security/url_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,16 @@ module UrlValidator
MAX_URL_LENGTH = 2048

class << self
# @param url [String]
# @return [String, nil]
def canonical_url(url)
normalize_url(url)
end

# @param url [String]
# @return [Boolean]
def valid_url?(url)
!normalize_url(url).nil?
!canonical_url(url).nil?
end

# @param account [Hash]
Expand All @@ -23,7 +29,7 @@ def url_allowed?(account, url)
return false unless account && url

allowed_urls = Array(account[:allowed_urls])
return false unless (normalized_url = normalize_url(url))
return false unless (normalized_url = canonical_url(url))

return false if allowed_urls.empty?

Expand All @@ -37,7 +43,7 @@ def url_allowed?(account, url)
def match_exact?(pattern, normalized_url)
return true if pattern == normalized_url

normalized_pattern = normalize_url(pattern)
normalized_pattern = canonical_url(pattern)
normalized_pattern ? normalized_pattern == normalized_url : false
end

Expand Down
10 changes: 5 additions & 5 deletions frontend/src/__tests__/App.contract.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ describe('App contract', () => {
const token = 'contract-token';

const authenticate = () => {
window.sessionStorage.setItem('html2rss_access_token', token);
window.localStorage.setItem('html2rss_access_token', token);
};

it('shows feed result when API responds with success', async () => {
Expand All @@ -18,7 +18,7 @@ describe('App contract', () => {
http.post('/api/v1/feeds', async ({ request }) => {
const body = (await request.json()) as { url: string; strategy: string };

expect(body).toEqual({ url: 'https://example.com/articles', strategy: 'browserless' });
expect(body).toEqual({ url: 'https://example.com/articles', strategy: 'faraday' });
expect(request.headers.get('authorization')).toBe(`Bearer ${token}`);

return HttpResponse.json(
Expand Down Expand Up @@ -55,7 +55,7 @@ describe('App contract', () => {

await screen.findByLabelText('Page URL');
await waitFor(() => {
expect(screen.getByRole('combobox')).toHaveValue('browserless');
expect(screen.getByRole('combobox')).toHaveValue('faraday');
});

const urlInput = screen.getByLabelText('Page URL') as HTMLInputElement;
Expand Down Expand Up @@ -149,7 +149,7 @@ describe('App contract', () => {

await screen.findByLabelText('Page URL');
await waitFor(() => {
expect(screen.getByRole('combobox')).toHaveValue('browserless');
expect(screen.getByRole('combobox')).toHaveValue('faraday');
});

fireEvent.input(screen.getByLabelText('Page URL'), {
Expand All @@ -161,6 +161,6 @@ describe('App contract', () => {

expect(screen.getByText('Add access token')).toBeInTheDocument();
expect(screen.queryByText('Feed generation failed')).not.toBeInTheDocument();
expect(window.sessionStorage.getItem('html2rss_access_token')).toBeNull();
expect(window.localStorage.getItem('html2rss_access_token')).toBeNull();
});
});
106 changes: 93 additions & 13 deletions frontend/src/__tests__/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,22 @@ describe('App', () => {
render(<App />);

expect(screen.getByLabelText('html2rss')).toBeInTheDocument();
expect(screen.getByRole('link', { name: 'html2rss' })).toHaveAttribute('href', '/');
expect(screen.getByLabelText('Page URL')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'More' })).toBeInTheDocument();
expect(screen.queryByRole('link', { name: 'Bookmarklet' })).not.toBeInTheDocument();
});

it('keeps the page url field permissive enough for hostname-only input', () => {
render(<App />);

const urlInput = screen.getByLabelText('Page URL');

expect(urlInput).toHaveAttribute('type', 'text');
expect(urlInput).toHaveAttribute('inputmode', 'url');
expect(urlInput).toHaveAttribute('autocapitalize', 'off');
});

it('autofocuses the source url field', async () => {
render(<App />);

Expand All @@ -104,11 +115,11 @@ describe('App', () => {
});
});

it('prefers browserless as the default strategy when available', () => {
it('prefers faraday as the default strategy when available', () => {
render(<App />);

return waitFor(() => {
expect(screen.getByRole('combobox')).toHaveValue('browserless');
expect(screen.getByRole('combobox')).toHaveValue('faraday');
});
});

Expand Down Expand Up @@ -140,11 +151,7 @@ describe('App', () => {
render(<App />);

await waitFor(() => {
expect(mockConvertFeed).toHaveBeenCalledWith(
'https://example.com/articles',
'browserless',
'saved-token'
);
expect(mockConvertFeed).toHaveBeenCalledWith('https://example.com/articles', 'faraday', 'saved-token');
});
});

Expand Down Expand Up @@ -221,7 +228,9 @@ describe('App', () => {
preview: {
items: [],
error: 'Preview unavailable right now.',
isLoading: false,
},
retry: null,
},
error: null,
convertFeed: mockConvertFeed,
Expand All @@ -243,6 +252,7 @@ describe('App', () => {
result: null,
error: 'Access denied',
convertFeed: mockConvertFeed,
clearError: mockClearConversionError,
clearResult: mockClearResult,
});

Expand Down Expand Up @@ -331,11 +341,7 @@ describe('App', () => {

await waitFor(() => {
expect(mockSaveToken).toHaveBeenCalledWith('token-123');
expect(mockConvertFeed).toHaveBeenCalledWith(
'https://example.com/articles',
'browserless',
'token-123'
);
expect(mockConvertFeed).toHaveBeenCalledWith('https://example.com/articles', 'faraday', 'token-123');
});
});

Expand Down Expand Up @@ -419,13 +425,87 @@ describe('App', () => {
expect(bookmarklet.getAttribute('href')).not.toContain('%27+encodeURIComponent');
});

it('opens token entry immediately for bookmarklet urls when no token is saved', async () => {
window.history.replaceState({}, '', 'http://localhost:3000/?url=example.com%2Farticles');

render(<App />);

await screen.findByText('Add access token');
expect(screen.getByLabelText('Page URL')).toHaveValue('https://example.com/articles');
expect(mockConvertFeed).not.toHaveBeenCalled();
});

it('offers a direct alternate strategy retry after conversion failure', async () => {
mockUseAccessToken.mockReturnValue({
token: 'saved-token',
hasToken: true,
saveToken: mockSaveToken,
clearToken: mockClearToken,
isLoading: false,
error: null,
});
mockConvertFeed
.mockRejectedValueOnce(
Object.assign(new Error('Tried faraday first, then browserless. Browserless failed.'), {
manualRetryStrategy: 'browserless',
})
)
.mockResolvedValueOnce(undefined);

render(<App />);

fireEvent.input(screen.getByLabelText('Page URL'), {
target: { value: 'https://example.com/articles' },
});
fireEvent.click(screen.getByRole('button', { name: 'Generate feed URL' }));

await screen.findByRole('button', { name: 'Try browserless instead' });
fireEvent.click(screen.getByRole('button', { name: 'Try browserless instead' }));

await waitFor(() => {
expect(mockConvertFeed).toHaveBeenLastCalledWith(
'https://example.com/articles',
'browserless',
'saved-token'
);
});
});

it('does not offer a duplicate retry action after automatic fallback already failed', async () => {
mockUseAccessToken.mockReturnValue({
token: 'saved-token',
hasToken: true,
saveToken: mockSaveToken,
clearToken: mockClearToken,
isLoading: false,
error: null,
});
mockConvertFeed.mockRejectedValueOnce(
Object.assign(new Error('Tried faraday first, then browserless. Browserless failed.'), {
manualRetryStrategy: '',
})
);

render(<App />);

fireEvent.input(screen.getByLabelText('Page URL'), {
target: { value: 'https://example.com/articles' },
});
fireEvent.click(screen.getByRole('button', { name: 'Generate feed URL' }));

await screen.findByText('Tried faraday first, then browserless. Browserless failed.');
expect(screen.queryByRole('button', { name: /Try .* instead/ })).not.toBeInTheDocument();
});

it('shows the utility links in a user-focused order', () => {
window.history.replaceState({}, '', 'http://localhost:3000/#result');
render(<App />);

fireEvent.click(screen.getByRole('button', { name: 'More' }));

const utilityLinks = screen.getAllByRole('link').map((link) => link.textContent);
const utilityLinks = Array.from(
screen.getByLabelText('Utilities').querySelectorAll('.utility-strip__items > a')
).map((link) => link.textContent);
expect(utilityLinks).toEqual([
'Try included feeds',
'Bookmarklet',
Expand Down
44 changes: 43 additions & 1 deletion frontend/src/__tests__/ResultDisplay.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ describe('ResultDisplay', () => {
},
],
error: null,
isLoading: false,
},
retry: null,
};

beforeEach(() => {
Expand All @@ -49,6 +51,10 @@ describe('ResultDisplay', () => {
expect(screen.getByText('Your feed is ready')).toBeInTheDocument();
expect(screen.getByText('Test Feed')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Copy feed URL' })).toBeInTheDocument();
expect(screen.getByRole('link', { name: 'Subscribe in reader' })).toHaveAttribute(
'href',
'feed:https://example.com/feed.xml'
);
expect(screen.getByRole('link', { name: 'Open feed' })).toBeInTheDocument();
expect(screen.getByRole('link', { name: 'Open JSON Feed' })).toHaveAttribute(
'href',
Expand All @@ -67,7 +73,10 @@ describe('ResultDisplay', () => {
it('surfaces preview failures as a result-state message', async () => {
render(
<ResultDisplay
result={{ ...mockResult, preview: { items: [], error: 'Preview unavailable right now.' } }}
result={{
...mockResult,
preview: { items: [], error: 'Preview unavailable right now.', isLoading: false },
}}
onCreateAnother={mockOnCreateAnother}
/>
);
Expand All @@ -78,6 +87,39 @@ describe('ResultDisplay', () => {
});
});

it('keeps the result state visible while preview is still loading', async () => {
render(
<ResultDisplay
result={{ ...mockResult, preview: { items: [], error: null, isLoading: true } }}
onCreateAnother={mockOnCreateAnother}
/>
);

await waitFor(() => {
expect(screen.getByText('Your feed is ready')).toBeInTheDocument();
expect(screen.getByRole('link', { name: 'Open feed' })).toBeInTheDocument();
expect(screen.getByText('Loading preview…')).toBeInTheDocument();
});
});

it('shows an automatic retry notice when fallback strategy succeeded', async () => {
render(
<ResultDisplay
result={{
...mockResult,
retry: { automatic: true, from: 'faraday', to: 'browserless' },
}}
onCreateAnother={mockOnCreateAnother}
/>
);

await waitFor(() => {
expect(
screen.getByText('Retried automatically with browserless after faraday could not finish the page.')
).toBeInTheDocument();
});
});

it('calls onCreateAnother when the reset button is clicked', () => {
render(<ResultDisplay result={mockResult} onCreateAnother={mockOnCreateAnother} />);

Expand Down
Loading
Loading