Skip to content

Commit 1fae998

Browse files
ClaudetaylortomCopilot
authored
Enhance architecture with security and performance improvements (#17)
* Initial plan * Implement security, robustness, and caching improvements - Replace manual query parsing with URL API - Remove Host header fallback, require baseUrl in config - Remove dead code: sanitizeHost, getProtocol, HOSTNAME_PATTERN - Add error metadata to API responses (items + errors array) - Update frontend to use data.items - Cap total aggregation time with configurable timeout - Handle config parse errors gracefully - Differentiate published vs created timestamps - Add ETag/If-None-Match support for API endpoints - Add security headers (X-Content-Type-Options, Referrer-Policy) Agent-Logs-Url: https://github.com/taylortom/newsflash/sessions/a0e166ae-d24b-4fe9-a149-18d31c765b99 Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com> * Fix timeout handling, prevent array mutations, and optimize ETag serialization - Clear aggregation timeout when fetch completes first - Clear per-feed timeouts in both success and error paths - Add timedOut flag to prevent duplicate errors after timeout - Clone results/errors arrays before caching to prevent post-response mutations - Optimize sendResponseWithETag to serialize JSON only once - Make RSS channel link and atom:link conditional on baseUrl presence Agent-Logs-Url: https://github.com/taylortom/newsflash/sessions/33ecba09-41d4-4a06-8add-b4b1e8fca93d Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com> --------- Co-authored-by: anthropic-code-agent[bot] <242468646+Claude@users.noreply.github.com> Co-authored-by: taylortom <1059083+taylortom@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 3f72ff1 commit 1fae998

4 files changed

Lines changed: 138 additions & 112 deletions

File tree

‎backend.js‎

Lines changed: 112 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,10 @@
11
import fs from 'fs/promises';
22
import http from 'http';
33
import path from 'path';
4+
import crypto from 'crypto';
45
import rssParser from './rss-parser.js';
56
import jsonToRss from './json-to-rss.js';
67

7-
// RFC-compliant hostname pattern:
8-
// - Must start and end with alphanumeric character
9-
// - Can contain alphanumeric, hyphens, and dots in between
10-
// - No consecutive dots, no leading/trailing dots or hyphens
11-
const HOSTNAME_PATTERN = /^[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9\-]*[a-zA-Z0-9])?)*$/;
12-
138
class Server {
149
constructor() {
1510
this._cache = {};
@@ -18,6 +13,10 @@ class Server {
1813
}
1914
async init() {
2015
await this.updateConfig();
16+
// Warn if baseUrl is not set
17+
if (!this.config.baseUrl) {
18+
console.warn('WARNING: config.baseUrl is not set. RSS feed links will be omitted. Set baseUrl in config.json for production use.');
19+
}
2120
this._http = http.createServer(async (req, res) => {
2221
try {
2322
await this.handleRequest(req, res);
@@ -29,15 +28,12 @@ class Server {
2928
console.log(`Server started listening on ${this.config.port}`);
3029
}
3130
parseQuery(req) {
32-
const queryString = req.url.split('?')[1] ?? '';
33-
const query = {};
31+
const url = new URL(req.url, 'http://localhost');
32+
const feeds = url.searchParams.get('feeds');
33+
const query = { feeds: 'default', limit: url.searchParams.get('limit'), since: url.searchParams.get('since') };
3434

35-
queryString.split('&').forEach(pair => {
36-
const [key, value] = pair.split('=');
37-
query[key] = value;
38-
});
39-
if(!query.feeds || !this.config.feeds[query.feeds]) {
40-
query.feeds = 'default';
35+
if(feeds && this.config.feeds[feeds]) {
36+
query.feeds = feeds;
4137
}
4238
return query;
4339
}
@@ -53,31 +49,33 @@ class Server {
5349
return this.sendResponse(res, { data: this._metrics });
5450
}
5551
if(req.method === 'GET' && req.url.startsWith('/api/news')) {
56-
return this.sendResponse(res, { data: await this.getRSS(query) });
52+
const data = await this.getRSS(query);
53+
return this.sendResponseWithETag(req, res, { data });
5754
}
5855
if(req.method === 'GET' && req.url.startsWith('/api/rss')) {
5956
const newsData = await this.getRSS(query);
60-
// Determine base URL for RSS feed links
61-
// RECOMMENDED: Set config.baseUrl in production for security and consistency
62-
// Fallback uses Host header (sanitized) and protocol detection
63-
// Note: Host header and X-Forwarded-Proto are client-controllable;
64-
// config.baseUrl should be used in production environments
65-
let baseUrl = this.config.baseUrl;
66-
if (!baseUrl) {
67-
const protocol = this.getProtocol(req);
68-
baseUrl = `${protocol}://${this.sanitizeHost(req.headers.host)}`;
69-
}
70-
const rssXml = jsonToRss.toRSS(newsData, {
57+
const rssXml = jsonToRss.toRSS(newsData.items, {
7158
title: this.config.name || 'Newsflash',
7259
description: 'Aggregated news feed',
73-
link: baseUrl
60+
link: this.config.baseUrl || ''
7461
});
75-
return this.sendResponse(res, { contentType: 'application/rss+xml', data: rssXml });
62+
return this.sendResponseWithETag(req, res, { contentType: 'application/rss+xml', data: rssXml });
7663
}
7764
await this.serveStatic(req, res);
7865
}
7966
async updateConfig() {
80-
this.config = JSON.parse(await fs.readFile('./config.json'))
67+
try {
68+
const configData = await fs.readFile('./config.json', 'utf-8');
69+
this.config = JSON.parse(configData);
70+
} catch(e) {
71+
if (!this.config) {
72+
// First load failed - exit with clear error message
73+
console.error('ERROR: Failed to load config.json:', e.message);
74+
process.exit(1);
75+
}
76+
// Keep previous config on subsequent failures
77+
console.error('ERROR: Failed to reload config.json, keeping previous configuration:', e.message);
78+
}
8179
}
8280
async getRSS(query) {
8381
await this.updateConfig();
@@ -88,25 +86,40 @@ class Server {
8886
return this._applyFilters(cached.data, query)
8987
}
9088
const results = [];
91-
await Promise.allSettled(this.config.feeds[query.feeds].map(f => {
89+
const errors = [];
90+
let timedOut = false;
91+
92+
// Cap total aggregation time
93+
const aggregationTimeout = (this.config.aggregationTimeout || 30) * 1000;
94+
const fetchPromise = Promise.allSettled(this.config.feeds[query.feeds].map(f => {
9295
return new Promise(async (resolve, reject) => {
93-
const t = setTimeout(() => {
94-
console.log(`RSS load timeout for ${f.feed}`);
96+
let timeoutHandle = setTimeout(() => {
97+
if (!timedOut) {
98+
const msg = `RSS load timeout for ${f.feed}`;
99+
console.log(msg);
100+
errors.push({ feed: f.name || f.feed, message: 'Request timeout' });
101+
}
95102
reject();
96-
}, this.config.timeout);
103+
}, this.config.timeout || 10000);
97104
const fetchStart = Date.now();
98105
try {
99106
const d = await rssParser.parse(f.feed);
100-
clearTimeout(t);
101-
if(d.items) results.push(...d.items.map(i => Object.assign(i, { feed: f.name ?? this.generateTitle(d), type: f.type ?? 'news' })));
107+
clearTimeout(timeoutHandle);
108+
if (!timedOut && d.items) {
109+
results.push(...d.items.map(i => Object.assign(i, { feed: f.name ?? this.generateTitle(d), type: f.type ?? 'news' })));
110+
}
102111
this._metrics[f.feed] = Object.assign(this._metrics[f.feed] || {}, {
103112
lastFetchMs: Date.now() - fetchStart,
104113
lastSuccess: Date.now(),
105114
failureCount: (this._metrics[f.feed]?.failureCount) || 0
106115
});
107116
} catch(e) {
108-
console.log(`Failed to parse ${f.feed}`, e.errno);
109-
console.log(e);
117+
clearTimeout(timeoutHandle);
118+
if (!timedOut) {
119+
console.log(`Failed to parse ${f.feed}`, e.errno);
120+
console.log(e);
121+
errors.push({ feed: f.name || f.feed, message: e.message || 'Parse error' });
122+
}
110123
this._metrics[f.feed] = Object.assign(this._metrics[f.feed] || {}, {
111124
lastFetchMs: Date.now() - fetchStart,
112125
lastFailure: Date.now(),
@@ -116,75 +129,49 @@ class Server {
116129
resolve();
117130
});
118131
}));
119-
const allData = results
132+
133+
// Race against aggregation timeout
134+
let aggregationTimeoutHandle;
135+
const timeoutPromise = new Promise((resolve) => {
136+
aggregationTimeoutHandle = setTimeout(() => {
137+
timedOut = true;
138+
errors.push({ feed: 'aggregation', message: `Aggregation timeout after ${this.config.aggregationTimeout || 30}s` });
139+
resolve();
140+
}, aggregationTimeout);
141+
});
142+
143+
await Promise.race([fetchPromise, timeoutPromise]);
144+
145+
// Clear aggregation timeout if fetch finished first
146+
if (aggregationTimeoutHandle) {
147+
clearTimeout(aggregationTimeoutHandle);
148+
}
149+
150+
// Clone results and errors to prevent post-response mutations
151+
const items = [...results]
120152
.sort((a, b) => {
121153
if(a.created < b.created) return 1;
122154
if(a.created > b.created) return -1;
123155
return 0;
124156
});
125-
this._cache[cacheKey] = { time: Date.now(), data: allData }
126-
return this._applyFilters(allData, query)
157+
const data = { items, errors: [...errors] };
158+
this._cache[cacheKey] = { time: Date.now(), data }
159+
return this._applyFilters(data, query)
127160
}
128161
_applyFilters(data, query) {
129162
const parsedLimit = parseInt(query.limit);
130163
const limit = (Number.isInteger(parsedLimit) && parsedLimit > 0) ? Math.min(parsedLimit, 100) : 100;
131164
const parsedSince = parseInt(query.since);
132165
const since = (Number.isInteger(parsedSince) && parsedSince > 0) ? parsedSince : null;
133-
let result = data;
166+
let filteredItems = data.items;
134167
if (since) {
135-
result = result.filter(i => i.created > since);
168+
filteredItems = filteredItems.filter(i => i.created > since);
136169
}
137-
return result.slice(0, limit);
170+
return { items: filteredItems.slice(0, limit), errors: data.errors };
138171
}
139172
generateTitle(data) {
140173
return data.title.split(/[^A-Za-z0-9\s]/)[0].trim();
141174
}
142-
getProtocol(req) {
143-
// Detect protocol from request (for proper RSS feed URLs)
144-
// Note: X-Forwarded-Proto is trusted by default, which is standard for proxy deployments
145-
// For production use, set config.baseUrl instead of relying on headers
146-
// Check X-Forwarded-Proto header (set by proxies/load balancers)
147-
const forwardedProto = req.headers['x-forwarded-proto'];
148-
if (forwardedProto === 'https') {
149-
return 'https';
150-
}
151-
// Check if connection is encrypted (direct HTTPS)
152-
if (req.socket && req.socket.encrypted) {
153-
return 'https';
154-
}
155-
// Default to http
156-
return 'http';
157-
}
158-
sanitizeHost(host) {
159-
// Validate and sanitize Host header to prevent injection attacks
160-
if (!host || typeof host !== 'string') {
161-
return `localhost:${this.config.port}`;
162-
}
163-
164-
// Reject hosts with multiple colons (invalid format)
165-
const colonCount = (host.match(/:/g) || []).length;
166-
if (colonCount > 1) {
167-
return `localhost:${this.config.port}`;
168-
}
169-
170-
// Split hostname and port
171-
const [hostname, portStr] = host.split(':');
172-
173-
// Validate hostname using RFC-compliant pattern
174-
if (!HOSTNAME_PATTERN.test(hostname)) {
175-
return `localhost:${this.config.port}`;
176-
}
177-
178-
// Validate port if specified
179-
if (portStr !== undefined) {
180-
const port = parseInt(portStr, 10);
181-
if (isNaN(port) || port < 1 || port > 65535) {
182-
return `localhost:${this.config.port}`;
183-
}
184-
}
185-
186-
return host;
187-
}
188175
async serveStatic(req, res) {
189176
const [url] = req.url.split('?');
190177
const filePath = url === '/' ? '/index.html' : url;
@@ -196,9 +183,42 @@ class Server {
196183
const fileExt = resolvedPath.slice(resolvedPath.lastIndexOf('.')+1);
197184
this.sendResponse(res, { contentType: this.extToMime(fileExt), data: await fs.readFile(resolvedPath) });
198185
}
199-
sendResponse(res, { statusCode=200, contentType='application/json', data }) {
200-
res.writeHead(statusCode, { 'Content-Type': contentType });
201-
res.end(contentType === 'application/json' ? JSON.stringify(data) : data, 'utf-8');
186+
sendResponse(res, { statusCode=200, contentType='application/json', data, headers={}, serializedBody=null }) {
187+
const baseHeaders = {
188+
'Content-Type': contentType,
189+
'X-Content-Type-Options': 'nosniff',
190+
'Referrer-Policy': 'strict-origin-when-cross-origin',
191+
...headers
192+
};
193+
res.writeHead(statusCode, baseHeaders);
194+
const body = serializedBody !== null ? serializedBody : (contentType === 'application/json' ? JSON.stringify(data) : data);
195+
res.end(body, 'utf-8');
196+
}
197+
sendResponseWithETag(req, res, { statusCode=200, contentType='application/json', data }) {
198+
// Serialize body once for both ETag and response
199+
const body = contentType === 'application/json' ? JSON.stringify(data) : data;
200+
const etag = '"' + crypto.createHash('md5').update(body).digest('hex').substring(0, 16) + '"';
201+
202+
// Check If-None-Match header
203+
const ifNoneMatch = req.headers['if-none-match'];
204+
if (ifNoneMatch === etag) {
205+
res.writeHead(304, {
206+
'ETag': etag,
207+
'X-Content-Type-Options': 'nosniff',
208+
'Referrer-Policy': 'strict-origin-when-cross-origin'
209+
});
210+
res.end();
211+
return;
212+
}
213+
214+
// Send response with ETag using pre-serialized body
215+
this.sendResponse(res, {
216+
statusCode,
217+
contentType,
218+
data,
219+
headers: { 'ETag': etag },
220+
serializedBody: body
221+
});
202222
}
203223
extToMime(ext) {
204224
switch(ext) {

‎json-to-rss.js‎

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,9 @@ function formatRFC822Date(timestamp) {
4444
* @param {Array} items - Array of news items with properties: title, description, link, published, created, author, id, feed
4545
* @param {Object} options - RSS feed options (title, description, link, language)
4646
* @returns {string} - RSS 2.0 XML string
47-
* @note Items without published/created timestamps will use Date.now() as fallback, which may produce
47+
* @note Items without published/created timestamps will use Date.now() as fallback, which may produce
4848
* misleading timestamps. This behavior matches the rss-parser.js fallback for consistency.
49+
* @note If options.link is not provided or is empty, channel <link> and atom:link will be omitted.
4950
*/
5051
export function toRSS(items, options = {}) {
5152
const {
@@ -54,14 +55,15 @@ export function toRSS(items, options = {}) {
5455
link = 'http://localhost:3000',
5556
language = 'en'
5657
} = options;
57-
58+
5859
const channelTitle = escapeXml(title);
5960
const channelDescription = escapeXml(description);
6061
const channelLink = escapeXml(link);
6162
const channelLanguage = escapeXml(language);
62-
63+
const hasLink = link && link.trim() !== '';
64+
6365
let rssItems = '';
64-
66+
6567
if (Array.isArray(items)) {
6668
rssItems = items.map(item => {
6769
const itemTitle = escapeXml(item.title || '');
@@ -78,44 +80,48 @@ export function toRSS(items, options = {}) {
7880
const guidValue = item.id || item.link || '';
7981
const itemGuid = escapeXml(guidValue);
8082
const isPermaLink = !!item.link && guidValue === item.link;
81-
83+
8284
let itemXml = ` <item>
8385
<title>${itemTitle}</title>
8486
<description>${itemDescription}</description>
8587
<link>${itemLink}</link>
8688
<pubDate>${itemPubDate}</pubDate>
8789
<guid isPermaLink="${isPermaLink ? 'true' : 'false'}">${itemGuid}</guid>`;
88-
90+
8991
if (itemAuthor) {
9092
itemXml += `\n <author>${itemAuthor}</author>`;
9193
}
92-
94+
9395
if (item.feed) {
9496
itemXml += `\n <category>${escapeXml(item.feed)}</category>`;
9597
}
96-
98+
9799
itemXml += '\n </item>';
98100
return itemXml;
99101
}).join('\n');
100102
}
101-
102-
// Construct atom:link self reference URL, handling trailing slashes
103-
const baseUrl = link.endsWith('/') ? link.slice(0, -1) : link;
104-
const selfLink = escapeXml(`${baseUrl}/api/rss`);
105-
103+
104+
// Construct channel link and atom:link only if baseUrl is provided
105+
let channelLinkElement = '';
106+
let atomLinkElement = '';
107+
if (hasLink) {
108+
channelLinkElement = `\n <link>${channelLink}</link>`;
109+
const baseUrl = link.endsWith('/') ? link.slice(0, -1) : link;
110+
const selfLink = escapeXml(`${baseUrl}/api/rss`);
111+
atomLinkElement = `\n <atom:link href="${selfLink}" rel="self" type="application/rss+xml" />`;
112+
}
113+
106114
const rss = `<?xml version="1.0" encoding="UTF-8"?>
107115
<rss version="2.0" xmlns:atom="http://www.w3.org/2005/Atom">
108116
<channel>
109117
<title>${channelTitle}</title>
110-
<description>${channelDescription}</description>
111-
<link>${channelLink}</link>
118+
<description>${channelDescription}</description>${channelLinkElement}
112119
<language>${channelLanguage}</language>
113-
<lastBuildDate>${formatRFC822Date(Date.now())}</lastBuildDate>
114-
<atom:link href="${selfLink}" rel="self" type="application/rss+xml" />
120+
<lastBuildDate>${formatRFC822Date(Date.now())}</lastBuildDate>${atomLinkElement}
115121
${rssItems}
116122
</channel>
117123
</rss>`;
118-
124+
119125
return rss;
120126
}
121127

‎public/js/news.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class Feed extends HTMLElement {
4040
this.shadowRoot.getElementById('items')?.remove();
4141

4242
const items = this.createEl({ type: 'div', attributes: { id: 'items', class: 'items' } });
43-
data.forEach(({ title, description, feed, created, link, type }) => {
43+
data.items.forEach(({ title, description, feed, created, link, type }) => {
4444
let extraHtml = '';
4545
if(feed === 'Hacker News') {
4646
extraHtml = `<a href="${description.match(`href="(.+)"`)[1]}" target="_blank">Comments</a>`;

0 commit comments

Comments
 (0)