Skip to content

Commit b5d036f

Browse files
marevolclaude
marevol
and
claude
authored
Improve Action classes: security enhancements, refactoring, and code quality improvements (#2961)
* Improve Action classes implementation - Add file path validation in FessAdminAction - Add URL validation in GoAction - Fix unsafe Optional.get() calls in LoginAction and ProfileAction - Refactor verifyCrudMode() to base class (31 files updated) - Extract permission encoding logic to base class helper method - Add audit logging for CRUD operations in User, Role, Group, and WebConfig actions - Improve error messages to be more specific * Fix Javadoc comment syntax errors * Fix Javadoc syntax in AdminGroupAction * Fix compilation errors in Action classes - Fix VaErrorHook type in verifyCrudMode method - Add StreamUtil.split static import - Make encodePermissions method static - Fix invalid error method in GoAction * Replace IllegalStateException with proper validation errors - Replace orElseThrow(IllegalStateException) with session validation - Redirect to login page when session is not found during password change - Use throwValidationError with addErrorsLoginError for missing session --------- Co-authored-by: Claude <[email protected]>
1 parent 2f1b2e6 commit b5d036f

29 files changed

+246
-507
lines changed

‎src/main/java/org/codelibs/fess/app/web/admin/accesstoken/AdminAccesstokenAction.java‎

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ public HtmlResponse createnew() {
208208
@Execute
209209
@Secured({ ROLE, ROLE + VIEW })
210210
public HtmlResponse details(final int crudMode, final String id) {
211-
verifyCrudMode(crudMode, CrudMode.DETAILS);
211+
verifyCrudMode(crudMode, CrudMode.DETAILS, this::asListHtml);
212212
saveToken();
213213
return asDetailsHtml().useForm(EditForm.class, op -> {
214214
op.setup(form -> {
@@ -272,7 +272,7 @@ public HtmlResponse edit(final EditForm form) {
272272
@Execute
273273
@Secured({ ROLE })
274274
public HtmlResponse create(final CreateForm form) {
275-
verifyCrudMode(form.crudMode, CrudMode.CREATE);
275+
verifyCrudMode(form.crudMode, CrudMode.CREATE, this::asListHtml);
276276
validate(form, messages -> {}, this::asEditHtml);
277277
verifyToken(this::asEditHtml);
278278
getAccessToken(form).ifPresent(entity -> {
@@ -299,7 +299,7 @@ public HtmlResponse create(final CreateForm form) {
299299
@Execute
300300
@Secured({ ROLE })
301301
public HtmlResponse update(final EditForm form) {
302-
verifyCrudMode(form.crudMode, CrudMode.EDIT);
302+
verifyCrudMode(form.crudMode, CrudMode.EDIT, this::asListHtml);
303303
validate(form, messages -> {}, this::asEditHtml);
304304
verifyToken(this::asEditHtml);
305305
getAccessToken(form).ifPresent(entity -> {
@@ -325,7 +325,7 @@ public HtmlResponse update(final EditForm form) {
325325
@Execute
326326
@Secured({ ROLE })
327327
public HtmlResponse delete(final EditForm form) {
328-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
328+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
329329
validate(form, messages -> {}, this::asDetailsHtml);
330330
verifyToken(this::asDetailsHtml);
331331
final String id = form.id;
@@ -383,31 +383,11 @@ public static OptionalEntity<AccessToken> getAccessToken(final CreateForm form)
383383
op -> op.exclude(Constants.COMMON_CONVERSION_RULE)
384384
.exclude(TOKEN, Constants.PERMISSIONS, EXPIRED_TIME)
385385
.dateConverter(Constants.DEFAULT_DATETIME_FORMAT, EXPIRES));
386-
final PermissionHelper permissionHelper = ComponentUtil.getPermissionHelper();
387-
entity.setPermissions(split(form.permissions, "\n").get(stream -> stream.map(s -> permissionHelper.encode(s))
388-
.filter(StringUtil::isNotBlank)
389-
.distinct()
390-
.toArray(n -> new String[n])));
386+
entity.setPermissions(encodePermissions(form.permissions));
391387
return entity;
392388
});
393389
}
394390

395-
// ===================================================================================
396-
// Small Helper
397-
// ============
398-
/**
399-
* Verify the CRUD mode.
400-
* @param crudMode The CRUD mode.
401-
* @param expectedMode The expected mode.
402-
*/
403-
protected void verifyCrudMode(final int crudMode, final int expectedMode) {
404-
if (crudMode != expectedMode) {
405-
throwValidationError(messages -> {
406-
messages.addErrorsCrudInvalidMode(GLOBAL, String.valueOf(expectedMode), String.valueOf(crudMode));
407-
}, this::asListHtml);
408-
}
409-
}
410-
411391
// ===================================================================================
412392
// JSP
413393
// =========

‎src/main/java/org/codelibs/fess/app/web/admin/badword/AdminBadwordAction.java‎

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ public HtmlResponse edit(final EditForm form) {
230230
@Execute
231231
@Secured({ ROLE, ROLE + VIEW })
232232
public HtmlResponse details(final int crudMode, final String id) {
233-
verifyCrudMode(crudMode, CrudMode.DETAILS);
233+
verifyCrudMode(crudMode, CrudMode.DETAILS, this::asListHtml);
234234
saveToken();
235235
return asDetailsHtml().useForm(EditForm.class, op -> {
236236
op.setup(form -> {
@@ -313,7 +313,7 @@ public HtmlResponse uploadpage() {
313313
@Execute
314314
@Secured({ ROLE })
315315
public HtmlResponse create(final CreateForm form) {
316-
verifyCrudMode(form.crudMode, CrudMode.CREATE);
316+
verifyCrudMode(form.crudMode, CrudMode.CREATE, this::asListHtml);
317317
validate(form, messages -> {}, this::asEditHtml);
318318
verifyToken(this::asEditHtml);
319319
getBadWord(form).ifPresent(entity -> {
@@ -340,7 +340,7 @@ public HtmlResponse create(final CreateForm form) {
340340
@Execute
341341
@Secured({ ROLE })
342342
public HtmlResponse update(final EditForm form) {
343-
verifyCrudMode(form.crudMode, CrudMode.EDIT);
343+
verifyCrudMode(form.crudMode, CrudMode.EDIT, this::asListHtml);
344344
validate(form, messages -> {}, this::asEditHtml);
345345
verifyToken(this::asEditHtml);
346346
getBadWord(form).ifPresent(entity -> {
@@ -367,7 +367,7 @@ public HtmlResponse update(final EditForm form) {
367367
@Execute
368368
@Secured({ ROLE })
369369
public HtmlResponse delete(final EditForm form) {
370-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
370+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
371371
validate(form, messages -> {}, this::asDetailsHtml);
372372
verifyToken(this::asDetailsHtml);
373373
final String id = form.id;
@@ -451,19 +451,6 @@ public static OptionalEntity<BadWord> getBadWord(final CreateForm form) {
451451
// ===================================================================================
452452
// Small Helper
453453
// ============
454-
/**
455-
* Verify the CRUD mode.
456-
* @param crudMode The CRUD mode.
457-
* @param expectedMode The expected mode.
458-
*/
459-
protected void verifyCrudMode(final int crudMode, final int expectedMode) {
460-
if (crudMode != expectedMode) {
461-
throwValidationError(messages -> {
462-
messages.addErrorsCrudInvalidMode(GLOBAL, String.valueOf(expectedMode), String.valueOf(crudMode));
463-
}, this::asListHtml);
464-
}
465-
}
466-
467454
private String getCsvEncoding() {
468455
return fessConfig.getCsvFileEncoding();
469456
}

‎src/main/java/org/codelibs/fess/app/web/admin/boostdoc/AdminBoostdocAction.java‎

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ public HtmlResponse edit(final EditForm form) {
208208
@Execute
209209
@Secured({ ROLE, ROLE + VIEW })
210210
public HtmlResponse details(final int crudMode, final String id) {
211-
verifyCrudMode(crudMode, CrudMode.DETAILS);
211+
verifyCrudMode(crudMode, CrudMode.DETAILS, this::asListHtml);
212212
saveToken();
213213
return asDetailsHtml().useForm(EditForm.class, op -> {
214214
op.setup(form -> {
@@ -235,7 +235,7 @@ public HtmlResponse details(final int crudMode, final String id) {
235235
@Execute
236236
@Secured({ ROLE })
237237
public HtmlResponse create(final CreateForm form) {
238-
verifyCrudMode(form.crudMode, CrudMode.CREATE);
238+
verifyCrudMode(form.crudMode, CrudMode.CREATE, this::asListHtml);
239239
validate(form, messages -> {}, this::asEditHtml);
240240
verifyToken(this::asEditHtml);
241241
getBoostDocumentRule(form).ifPresent(entity -> {
@@ -261,7 +261,7 @@ public HtmlResponse create(final CreateForm form) {
261261
@Execute
262262
@Secured({ ROLE })
263263
public HtmlResponse update(final EditForm form) {
264-
verifyCrudMode(form.crudMode, CrudMode.EDIT);
264+
verifyCrudMode(form.crudMode, CrudMode.EDIT, this::asListHtml);
265265
validate(form, messages -> {}, this::asEditHtml);
266266
verifyToken(this::asEditHtml);
267267
getBoostDocumentRule(form).ifPresent(entity -> {
@@ -287,7 +287,7 @@ public HtmlResponse update(final EditForm form) {
287287
@Execute
288288
@Secured({ ROLE })
289289
public HtmlResponse delete(final EditForm form) {
290-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
290+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
291291
validate(form, messages -> {}, this::asDetailsHtml);
292292
verifyToken(this::asDetailsHtml);
293293
final String id = form.id;
@@ -346,22 +346,6 @@ public static OptionalEntity<BoostDocumentRule> getBoostDocumentRule(final Creat
346346
});
347347
}
348348

349-
// ===================================================================================
350-
// Small Helper
351-
// ============
352-
/**
353-
* Verify the CRUD mode.
354-
* @param crudMode The CRUD mode.
355-
* @param expectedMode The expected mode.
356-
*/
357-
protected void verifyCrudMode(final int crudMode, final int expectedMode) {
358-
if (crudMode != expectedMode) {
359-
throwValidationError(messages -> {
360-
messages.addErrorsCrudInvalidMode(GLOBAL, String.valueOf(expectedMode), String.valueOf(crudMode));
361-
}, this::asListHtml);
362-
}
363-
}
364-
365349
// ===================================================================================
366350
// JSP
367351
// =========

‎src/main/java/org/codelibs/fess/app/web/admin/crawlinginfo/AdminCrawlinginfoAction.java‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ protected void searchPaging(final RenderData data, final SearchForm form) {
177177
@Execute
178178
@Secured({ ROLE, ROLE + VIEW })
179179
public HtmlResponse details(final int crudMode, final String id) {
180-
verifyCrudMode(crudMode, CrudMode.DETAILS);
180+
verifyCrudMode(crudMode, CrudMode.DETAILS, this::asListHtml);
181181
saveToken();
182182
return crawlingInfoService.getCrawlingInfo(id)
183183
.map(entity -> asHtml(path_AdminCrawlinginfo_AdminCrawlinginfoDetailsJsp).useForm(EditForm.class, op -> {
@@ -205,7 +205,7 @@ public HtmlResponse details(final int crudMode, final String id) {
205205
@Execute
206206
@Secured({ ROLE })
207207
public HtmlResponse threaddump(final EditForm form) {
208-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
208+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
209209
validate(form, messages -> {}, this::asDetailsHtml);
210210
verifyToken(this::asDetailsHtml);
211211
final String id = form.id;
@@ -234,7 +234,7 @@ public HtmlResponse threaddump(final EditForm form) {
234234
@Execute
235235
@Secured({ ROLE })
236236
public HtmlResponse delete(final EditForm form) {
237-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
237+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
238238
validate(form, messages -> {}, this::asDetailsHtml);
239239
verifyToken(this::asDetailsHtml);
240240
final String id = form.id;
@@ -266,19 +266,6 @@ public HtmlResponse deleteall() {
266266
// ===================================================================================
267267
// Small Helper
268268
// ============
269-
/**
270-
* Verify the CRUD mode.
271-
* @param crudMode The CRUD mode.
272-
* @param expectedMode The expected mode.
273-
*/
274-
protected void verifyCrudMode(final int crudMode, final int expectedMode) {
275-
if (crudMode != expectedMode) {
276-
throwValidationError(messages -> {
277-
messages.addErrorsCrudInvalidMode(GLOBAL, String.valueOf(expectedMode), String.valueOf(crudMode));
278-
}, this::asListHtml);
279-
}
280-
}
281-
282269
// ===================================================================================
283270
// JSP
284271
// =========

‎src/main/java/org/codelibs/fess/app/web/admin/dataconfig/AdminDataconfigAction.java‎

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ public HtmlResponse edit(final EditForm form) {
253253
@Execute
254254
@Secured({ ROLE, ROLE + VIEW })
255255
public HtmlResponse details(final int crudMode, final String id) {
256-
verifyCrudMode(crudMode, CrudMode.DETAILS);
256+
verifyCrudMode(crudMode, CrudMode.DETAILS, this::asListHtml);
257257
saveToken();
258258
return asDetailsHtml().useForm(EditForm.class, op -> {
259259
op.setup(form -> {
@@ -288,7 +288,7 @@ public HtmlResponse details(final int crudMode, final String id) {
288288
@Execute
289289
@Secured({ ROLE })
290290
public HtmlResponse create(final CreateForm form) {
291-
verifyCrudMode(form.crudMode, CrudMode.CREATE);
291+
verifyCrudMode(form.crudMode, CrudMode.CREATE, this::asListHtml);
292292
validate(form, messages -> {}, this::asEditHtml);
293293
verifyToken(this::asEditHtml);
294294
getDataConfig(form).ifPresent(entity -> {
@@ -314,7 +314,7 @@ public HtmlResponse create(final CreateForm form) {
314314
@Execute
315315
@Secured({ ROLE })
316316
public HtmlResponse update(final EditForm form) {
317-
verifyCrudMode(form.crudMode, CrudMode.EDIT);
317+
verifyCrudMode(form.crudMode, CrudMode.EDIT, this::asListHtml);
318318
validate(form, messages -> {}, this::asEditHtml);
319319
verifyToken(this::asEditHtml);
320320
getDataConfig(form).ifPresent(entity -> {
@@ -340,7 +340,7 @@ public HtmlResponse update(final EditForm form) {
340340
@Execute
341341
@Secured({ ROLE })
342342
public HtmlResponse delete(final EditForm form) {
343-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
343+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
344344
validate(form, messages -> {}, this::asDetailsHtml);
345345
verifyToken(this::asDetailsHtml);
346346
final String id = form.id;
@@ -402,11 +402,7 @@ public static OptionalEntity<DataConfig> getDataConfig(final CreateForm form) {
402402
op -> op.exclude(Stream
403403
.concat(Stream.of(Constants.COMMON_CONVERSION_RULE), Stream.of(Constants.PERMISSIONS, Constants.VIRTUAL_HOSTS))
404404
.toArray(n -> new String[n])));
405-
final PermissionHelper permissionHelper = ComponentUtil.getPermissionHelper();
406-
entity.setPermissions(split(form.permissions, "\n").get(stream -> stream.map(s -> permissionHelper.encode(s))
407-
.filter(StringUtil::isNotBlank)
408-
.distinct()
409-
.toArray(n -> new String[n])));
405+
entity.setPermissions(encodePermissions(form.permissions));
410406
entity.setVirtualHosts(split(form.virtualHosts, "\n")
411407
.get(stream -> stream.filter(StringUtil::isNotBlank).distinct().map(String::trim).toArray(n -> new String[n])));
412408
return entity;
@@ -440,19 +436,6 @@ protected void registerHandlerNames(final RenderData data) {
440436
// ===================================================================================
441437
// Small Helper
442438
// ============
443-
/**
444-
* Verify the CRUD mode.
445-
* @param crudMode The CRUD mode.
446-
* @param expectedMode The expected mode.
447-
*/
448-
protected void verifyCrudMode(final int crudMode, final int expectedMode) {
449-
if (crudMode != expectedMode) {
450-
throwValidationError(messages -> {
451-
messages.addErrorsCrudInvalidMode(GLOBAL, String.valueOf(expectedMode), String.valueOf(crudMode));
452-
}, this::asListHtml);
453-
}
454-
}
455-
456439
// ===================================================================================
457440
// JSP
458441
// =========

‎src/main/java/org/codelibs/fess/app/web/admin/duplicatehost/AdminDuplicatehostAction.java‎

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ public HtmlResponse edit(final EditForm form) {
217217
@Execute
218218
@Secured({ ROLE, ROLE + VIEW })
219219
public HtmlResponse details(final int crudMode, final String id) {
220-
verifyCrudMode(crudMode, CrudMode.DETAILS);
220+
verifyCrudMode(crudMode, CrudMode.DETAILS, this::asListHtml);
221221
saveToken();
222222
return asHtml(path_AdminDuplicatehost_AdminDuplicatehostDetailsJsp).useForm(EditForm.class, op -> {
223223
op.setup(form -> {
@@ -245,7 +245,7 @@ public HtmlResponse details(final int crudMode, final String id) {
245245
@Execute
246246
@Secured({ ROLE })
247247
public HtmlResponse create(final CreateForm form) {
248-
verifyCrudMode(form.crudMode, CrudMode.CREATE);
248+
verifyCrudMode(form.crudMode, CrudMode.CREATE, this::asListHtml);
249249
validate(form, messages -> {}, this::asEditHtml);
250250
verifyToken(this::asEditHtml);
251251
getDuplicateHost(form).ifPresent(entity -> {
@@ -272,7 +272,7 @@ public HtmlResponse create(final CreateForm form) {
272272
@Execute
273273
@Secured({ ROLE })
274274
public HtmlResponse update(final EditForm form) {
275-
verifyCrudMode(form.crudMode, CrudMode.EDIT);
275+
verifyCrudMode(form.crudMode, CrudMode.EDIT, this::asListHtml);
276276
validate(form, messages -> {}, this::asEditHtml);
277277
verifyToken(this::asEditHtml);
278278
getDuplicateHost(form).ifPresent(entity -> {
@@ -299,7 +299,7 @@ public HtmlResponse update(final EditForm form) {
299299
@Execute
300300
@Secured({ ROLE })
301301
public HtmlResponse delete(final EditForm form) {
302-
verifyCrudMode(form.crudMode, CrudMode.DETAILS);
302+
verifyCrudMode(form.crudMode, CrudMode.DETAILS, this::asListHtml);
303303
validate(form, messages -> {}, this::asDetailsHtml);
304304
verifyToken(this::asDetailsHtml);
305305
final String id = form.id;
@@ -366,23 +366,6 @@ public static OptionalEntity<DuplicateHost> getDuplicateHost(final CreateForm fo
366366
});
367367
}
368368

369-
// ===================================================================================
370-
// Small Helper
371-
// ============
372-
/**
373-
* Verifies that the CRUD mode matches the expected mode.
374-
*
375-
* @param crudMode the actual CRUD mode
376-
* @param expectedMode the expected CRUD mode
377-
*/
378-
protected void verifyCrudMode(final int crudMode, final int expectedMode) {
379-
if (crudMode != expectedMode) {
380-
throwValidationError(messages -> {
381-
messages.addErrorsCrudInvalidMode(GLOBAL, String.valueOf(expectedMode), String.valueOf(crudMode));
382-
}, this::asListHtml);
383-
}
384-
}
385-
386369
// ===================================================================================
387370
// JSP
388371
// =========

0 commit comments

Comments
 (0)