Skip to content

Commit ca1bdeb

Browse files
committed
Follow-up: replace the regex-based 401/403 heuristic in
`mapIpfsError` with an explicit `httpStatus` field so the classification is data-driven and the rate-limit / file-size / token failures each route to the correct RFC 7807 catalog row. **Breaking**: rate-limit failures now map to `429 RATE_LIMITED` instead of the previous mis-classified `401 UNAUTHORIZED`. Callers that relied on the heuristic artifact (the rate-limit assertion in `tests/middleware/auth.test.js` was migrated that way during the first PR) are updated to expect `RateLimitError`/429. **Backward compatible**: `createIpfsError(message, operation, details)` still works \u2014 the new 4th param `httpStatus` defaults to `null` and `mapIpfsError` falls back to a frozen dispatch table keyed by `operation`. All 25 other `createIpfsError` call-sites in `services/ipfs.js` and `routes/content.js` continue to compile and resolve to the same RFC 7807 status they did before (auth \u2192 401, validation \u2192 400, init \u2192 503, get* \u2192 404, upload/pin* \u2192 500). **Other fixes folded in**: - `validateFileSize` no longer blanket-maps every IPFS error to 413. File-size overruns stay 413; other ipfs validation failures bubble up as 400 unless overridden. - `optionalIpfsAuth` silent-fallback for invalid tokens / unexpected errors preserved exactly \u2014 the only error class that surfaces is explicit 403, no more `/permission/i` regex. - `OPERATION_STATUS_MAP` exported for unit testing. **Tests** (`backend/tests/utils/ipfsUtils.test.js`): - `createIpfsError` 3-arg / 4-arg backward-compat - `mapIpfsError` dispatch for each documented status (400/401/403/404/\n413/429/503) - Fallback-table coverage for every operation in `OPERATION_STATUS_MAP`\n- Unknown status / unknown operation \u2192 `InternalError(500)`\n- Non-Ipfs / null / undefined \u2192 `AuthError(401)` preserved\n\n`backend/tests/middleware/auth.test.js` updated:\n- Rate-limit assertion moved from stale `AuthError(401)` \u2192\n `RateLimitError(429)` (matches the new contract).\n- Over-engineered `validateFileSize` test (jest.resetModules +\n `process.env` overrides) replaced with a clean `jest.isolateModules`\n + `jest.doMock` block asserting the payload AppError class.\n- `optionalIpfsAuth` `ForbiddenError` test replaced with a real\n `supertest(app).get('/')` drive against `optionalIpfsAuth \u2192\n errorHandler` asserting `res.body.code === 'FORBIDDEN'` and\n `res.headers['content-type'] === 'application/problem+json'`.\n\nRefs: #254, #127.
1 parent 96af25b commit ca1bdeb

4 files changed

Lines changed: 431 additions & 117 deletions

File tree

backend/src/middleware/ipfsAuth.js

Lines changed: 166 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,93 @@
11
const jwt = require('jsonwebtoken');
22
const { ipfsConfig } = require('../config/ipfs');
33
const { createIpfsError } = require('../utils/ipfsUtils');
4-
const { AuthError, ForbiddenError, ValidationError, PayloadTooLargeError } = require('../utils/errors');
4+
const {
5+
AuthError,
6+
ForbiddenError,
7+
ValidationError,
8+
PayloadTooLargeError,
9+
RateLimitError,
10+
NotFoundError,
11+
ServiceUnavailableError,
12+
InternalError,
13+
} = require('../utils/errors');
14+
15+
/**
16+
* Operation → HTTP status fallback table, applied only when an IPFS error
17+
* is constructed without an explicit {@link createIpfsError} `httpStatus`.
18+
* Each row maps the IPFS operation name (e.g. `'auth'`, `'validation'`) to
19+
* the status most appropriate for the RFC 7807 envelope.
20+
*
21+
* Callers that want a different status than the operation default MUST pass
22+
* the desired status as the 4th argument to `createIpfsError`.
23+
*/
24+
const OPERATION_STATUS_MAP = Object.freeze({
25+
auth: 401,
26+
validation: 400,
27+
init: 503,
28+
getContent: 404,
29+
getMetadata: 404,
30+
upload: 500,
31+
pinContent: 500,
32+
unpinContent: 500,
33+
getNodeInfo: 500,
34+
rateLimit: 429,
35+
});
536

637
/**
738
* Map an IPFS-domain error (created via {@link createIpfsError}) onto the
8-
* canonical RFC 7807 AppError family. Unknown errors are re-wrapped as
9-
* an AuthError because the IPFS auth middleware only emits authentication-
10-
* shaped failures.
39+
* canonical RFC 7807 AppError family. Classification is now data-driven:
40+
* the explicit `httpStatus` set at the throw site wins; otherwise the
41+
* {@link OPERATION_STATUS_MAP} fallback determines the status. Message text
42+
* is never inspected.
43+
*
44+
* @param {Error|null|undefined} error - The thrown value.
45+
* @returns {import('../utils/errors').AppError} The matching inherited
46+
* `AppError` so the central error handler emits the right envelope.
1147
*/
12-
const mapIpfsError = (error, operation) => {
48+
const mapIpfsError = (error) => {
1349
if (!error || !error.isIpfsError) {
1450
return new AuthError('Authentication failed');
1551
}
16-
const message = error.message || 'Authentication failed';
17-
if (error.operation === 'validation') {
18-
const err = new ValidationError(message);
19-
err.details = error.details;
20-
return err;
52+
53+
const status =
54+
typeof error.httpStatus === 'number'
55+
? error.httpStatus
56+
: OPERATION_STATUS_MAP[error.operation] || 500;
57+
const message = error.message || 'IPFS operation failed';
58+
59+
let appError;
60+
switch (status) {
61+
case 400:
62+
appError = new ValidationError(message);
63+
break;
64+
case 401:
65+
appError = new AuthError(message);
66+
break;
67+
case 403:
68+
appError = new ForbiddenError(message);
69+
break;
70+
case 404:
71+
appError = new NotFoundError(message);
72+
break;
73+
case 413:
74+
appError = new PayloadTooLargeError(message);
75+
break;
76+
case 429:
77+
appError = new RateLimitError(message);
78+
break;
79+
case 503:
80+
appError = new ServiceUnavailableError(message);
81+
break;
82+
default:
83+
appError = new InternalError(message);
84+
break;
2185
}
22-
// auth-shaped errors: distinguish 401 (missing/invalid token) vs 403
23-
// (insufficient permissions) using the message as the heuristic since
24-
// the IPFS layer does not currently classify these. This still emits
25-
// a stable RFC 7807 envelope end-to-end.
26-
if (/insufficient/i.test(message) || /permission/i.test(message)) {
27-
const err = new ForbiddenError(message);
28-
err.details = error.details;
29-
return err;
86+
87+
if (error.details !== undefined) {
88+
appError.details = error.details;
3089
}
31-
const err = new AuthError(message);
32-
err.details = error.details;
33-
return err;
90+
return appError;
3491
};
3592

3693
/**
@@ -47,7 +104,12 @@ const verifyToken = (token) => {
47104
try {
48105
return jwt.verify(token, process.env.JWT_SECRET);
49106
} catch (error) {
50-
throw createIpfsError('Invalid or expired token', 'auth', { error: error.message });
107+
throw createIpfsError(
108+
'Invalid or expired token',
109+
'auth',
110+
{ error: error.message },
111+
401,
112+
);
51113
}
52114
};
53115

@@ -101,18 +163,26 @@ const checkRateLimit = (user, operation) => {
101163
};
102164

103165
const userLimit = rateLimits[operation]?.[user.role] || 0;
104-
166+
105167
// For demo purposes, we'll use a simple in-memory counter
106168
// In production, use Redis or similar for distributed rate limiting
107169
const userKey = `${user.id}:${operation}`;
108170
const currentCount = global.ipfsRateLimit?.[userKey] || 0;
109-
171+
110172
if (currentCount >= userLimit) {
111-
throw createIpfsError('Rate limit exceeded', 'auth', {
112-
operation,
113-
limit: userLimit,
114-
current: currentCount
115-
});
173+
// 429 Rate Limit is the canonical status for quota breaches — set it
174+
// explicitly rather than letting the auth fallback mis-classify this
175+
// as 401 Unauthorized.
176+
throw createIpfsError(
177+
'Rate limit exceeded',
178+
'auth',
179+
{
180+
operation,
181+
limit: userLimit,
182+
current: currentCount,
183+
},
184+
429,
185+
);
116186
}
117187

118188
// Increment counter (reset every hour)
@@ -141,22 +211,33 @@ const ipfsAuth = (operation = 'download') => {
141211
try {
142212
// Extract token from Authorization header
143213
const authHeader = req.headers.authorization;
144-
214+
145215
if (!authHeader || !authHeader.startsWith('Bearer ')) {
146-
throw createIpfsError('Authorization token required', 'auth');
216+
throw createIpfsError(
217+
'Authorization token required',
218+
'auth',
219+
undefined,
220+
401,
221+
);
147222
}
148223

149224
const token = authHeader.substring(7);
150-
225+
151226
// Verify token and extract user
152227
const user = verifyToken(token);
153228

154229
// Check if user has required permissions
155230
if (!hasPermission(user, operation)) {
156-
throw createIpfsError('Insufficient permissions for this operation', 'auth', {
157-
operation,
158-
userRole: user.role
159-
});
231+
// Explicit 403 — does NOT rely on message-text heuristics.
232+
throw createIpfsError(
233+
'Insufficient permissions for this operation',
234+
'auth',
235+
{
236+
operation,
237+
userRole: user.role,
238+
},
239+
403,
240+
);
160241
}
161242

162243
// Check rate limits
@@ -168,7 +249,7 @@ const ipfsAuth = (operation = 'download') => {
168249

169250
next();
170251
} catch (error) {
171-
return next(mapIpfsError(error, operation));
252+
return next(mapIpfsError(error));
172253
}
173254
};
174255
};
@@ -188,10 +269,16 @@ const optionalIpfsAuth = (operation = 'download') => {
188269

189270
// Check permissions if user is authenticated
190271
if (!hasPermission(user, operation)) {
191-
throw createIpfsError('Insufficient permissions for this operation', 'auth', {
192-
operation,
193-
userRole: user.role
194-
});
272+
// Explicit 403 — surface as a forbidden response.
273+
throw createIpfsError(
274+
'Insufficient permissions for this operation',
275+
'auth',
276+
{
277+
operation,
278+
userRole: user.role,
279+
},
280+
403,
281+
);
195282
}
196283

197284
// Check rate limits for authenticated users
@@ -203,17 +290,22 @@ const optionalIpfsAuth = (operation = 'download') => {
203290
req.ipfsOperation = operation;
204291
next();
205292
} catch (error) {
293+
// Check the explicit/routed status instead of message-text. Only
294+
// 403 (permission denial) is surfaced; other failures (401 invalid
295+
// token, 429 rate limit, 500 unexpected) silently degrade to a
296+
// no-user request, preserving the prior "optional auth" semantics.
206297
if (error && error.isIpfsError) {
207-
// Permission failures are surfaced; invalid tokens for optional
208-
// auth fall through without attaching a user identity (preserved
209-
// pre-migration behaviour).
210-
if (/permission/i.test(error.message || '')) {
211-
return next(mapIpfsError(error, operation));
298+
const status =
299+
typeof error.httpStatus === 'number'
300+
? error.httpStatus
301+
: OPERATION_STATUS_MAP[error.operation];
302+
if (status === 403) {
303+
return next(mapIpfsError(error));
212304
}
213305
req.ipfsOperation = operation;
214306
return next();
215307
}
216-
// Unknown errors also fall through silently for optional auth.
308+
// Unknown / non-IPFS errors also fall through silently.
217309
req.ipfsOperation = operation;
218310
next();
219311
}
@@ -245,7 +337,7 @@ const validateContentAccess = async (req, res, next) => {
245337
// 1. Check if the user is the content owner
246338
// 2. Check if the content is shared with the user
247339
// 3. Check if the content is part of a course the user is enrolled in
248-
340+
249341
// For demo purposes, we'll allow access
250342
next();
251343
} catch (error) {
@@ -255,23 +347,37 @@ const validateContentAccess = async (req, res, next) => {
255347

256348
/**
257349
* File size validation middleware
258-
* Validates file size before upload
350+
* Validates file size before upload.
351+
*
352+
* Sets an explicit `httpStatus: 413` on the IPFS error it raises so
353+
* {@link mapIpfsError} produces a `PayloadTooLargeError` envelope instead
354+
* of the previous blanket-mapping that mis-classified every IPFS error as
355+
* 413 (Issue #254 follow-up).
259356
*/
260357
const validateFileSize = (req, res, next) => {
261358
try {
262359
if (req.file && req.file.size > ipfsConfig.maxFileSize) {
263-
throw createIpfsError('File size exceeds maximum limit', 'validation', {
264-
maxSize: ipfsConfig.maxFileSize,
265-
actualSize: req.file.size
266-
});
360+
// Explicit 413 — drives the matching AppError subclass through
361+
// `mapIpfsError` rather than relying on the validation fallback.
362+
throw createIpfsError(
363+
'File size exceeds maximum limit',
364+
'validation',
365+
{
366+
maxSize: ipfsConfig.maxFileSize,
367+
actualSize: req.file.size,
368+
},
369+
413,
370+
);
267371
}
268372

269373
next();
270374
} catch (error) {
271375
if (error && error.isIpfsError) {
272-
// File-size overruns surface as a Pay-load Too Large problem
273-
// (RFC 7807) so the existing error catalog row matches the wire.
274-
return next(new PayloadTooLargeError(error.message || 'File size exceeds maximum limit'));
376+
// Use mapIpfsError so the status field drives the AppError
377+
// selection — file-size overruns become 413 PayloadTooLargeError
378+
// and other ipfs validation failures bubble up as 400 ValidationError
379+
// (or whatever the throw site specified).
380+
return next(mapIpfsError(error));
275381
}
276382
return next(new ValidationError('File validation failed'));
277383
}
@@ -284,5 +390,8 @@ module.exports = {
284390
validateFileSize,
285391
verifyToken,
286392
hasPermission,
287-
checkRateLimit
393+
checkRateLimit,
394+
// Exported for unit tests; consumers should normally use `mapIpfsError`.
395+
mapIpfsError,
396+
OPERATION_STATUS_MAP,
288397
};

backend/src/utils/ipfsUtils.js

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -191,16 +191,27 @@ const parseCid = (cid) => {
191191
};
192192

193193
/**
194-
* Create error object with IPFS-specific information
195-
* @param {string} message - The error message
194+
* Create error object with IPFS-specific information.
195+
*
196+
* The optional `httpStatus` parameter is the canonical RFC 7807 status
197+
* the caller wants surfaced on the wire. When omitted, the value is
198+
* `null` and downstream consumers (e.g. `mapIpfsError`) fall back to a
199+
* table-driven default derived from `operation`. Setting it explicitly
200+
* removes any need for message-text heuristics on the consumer side.
201+
*
202+
* @param {string} message - The error message
196203
* @param {string} operation - The operation that failed
197-
* @param {Object} details - Additional error details
198-
* @returns {Error} - The formatted error
204+
* @param {Object} [details={}] - Additional error details
205+
* @param {number|null} [httpStatus=null] - Optional explicit HTTP status
206+
* code (400/401/403/404/413/429/500/503). Pass `null` to defer to the
207+
* operation-based default.
208+
* @returns {Error} - The formatted error (plain Error, not AppError)
199209
*/
200-
const createIpfsError = (message, operation, details = {}) => {
210+
const createIpfsError = (message, operation, details = {}, httpStatus = null) => {
201211
const error = new Error(message);
202212
error.operation = operation;
203213
error.details = details;
214+
error.httpStatus = httpStatus;
204215
error.isIpfsError = true;
205216
return error;
206217
};

0 commit comments

Comments
 (0)